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 <b...@mariadb.com > <mailto:b...@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 > _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp