Re: [Maria-developers] Please review a patch for MDEV-15758 Split Item_bool_func::get_mm_leaf() into virtual methods in Field and Type_handler

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 mailto:bar@mariadb.com> 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@lists.launchpad.net mailto:maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp