maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

Hi Alexander!

(I was doing some experimentation with the code and forgot to send the
email properly)

So the main things we discussed during the review:

We discussed alternative of not adding SEL_ARG_XXX classes and instead have
separate constructors within SEL_ARG. I tried to do this myself but I am
not happy with the results. So in this case, let's add extra comments for
SEL_ARG_XXX classes, explaining their purpose as Sergey Petrunia mentioned.

Check DBUG_ENTER strings. Some don't match the actual function name.

The patch in whole looks good, however it took me quite a bit of time to
understand where the extra logic is added and where it's just part of
refactoring. If feasible, with least amount of effort, split the patch up
into 2 pieces, one that only does refactoring and one that introduces the
extra optimization code (plus small refactoring to get it to work). The
specific function I'm looking at
is: Field::stored_field_make_mm_leaf_bounded_int

This will make it a lot easier when tracking history, in case new bugs
arise to understand the scope of the change much quicker. The first patch
should not require any test result changes at all, while the second one
should highlight the optimizations now happening. If for some reason the
type system gets smarter just through the refactoring part, then we update
test results there, but from what I understand from the patch, that should
not be the case. If it does change the result, maybe we should discuss this
more as it means I missed something.

Within stored_field_cmp_to_item, the long if statement logic about various
field types that has now been split within the type system, I like that
bit. I did step through the code for quite some time and it seems like
perfectly fine logic, but the mechanism takes a while to grasp and I am not
100% confident I remember or understood everything. For the scope of this
review, I was satisfied with what I could figure out.

Thanks for your patience in receiving this review.
Vicențiu

On Mon, 9 Jul 2018 at 11:03 Alexander Barkov <bar@xxxxxxxxxxx> wrote:

> Hi Vicențiu,
>
> Sending the patch adjusted to the latest 10.4.
>
> Thanks.
>
>
> On 04/03/2018 12:57 PM, Alexander Barkov wrote:
> > Hello Vicentiu,
> >
> > Please review my patch for MDEV-15758.
> >
> > It also fixes this bug:
> >
> > MDEV-15759 Expect "Impossible WHERE" for
> > indexed_int_column=out_of_range_int_constant
> >
> > Thanks!
> >
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp
>