maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Vicențiu

Thanks for your review! Please find my comments inline:

On 07/18/2018 04:38 PM, Vicențiu Ciorbaru wrote:
> 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.

I added more comments.

Btw, SEL_ARG() itself does not have any comments :)

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

Fixed.

> 
> 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.

I moved changes for "MDEV-15759 Expect "Impossible WHERE" for
indexed_int_column=out_of_range_int_constant" into a separate patch.

> 
> 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.

Thanks!

Both MDEV-15759 and MDEV-15758 are now pushed to 10.4.



> Vicențiu
> 
> On Mon, 9 Jul 2018 at 11:03 Alexander Barkov <bar@xxxxxxxxxxx
> <mailto: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
>     <mailto:maria-developers@xxxxxxxxxxxxxxxxxxx>
>     Unsubscribe : https://launchpad.net/~maria-developers
>     More help   : https://help.launchpad.net/ListHelp
>