In-Reply-To: <[email protected]> Hi Alexey,
Thanks for your patience in waiting for the review. Please find it below. On Wed, Sep 28, 2016 at 02:50:19PM +0400, Alexey Botchkov wrote: > revision-id: 41a12f990519fb68eaa66ecc6860985471e6ba5a > (mariadb-10.1.8-264-g41a12f9) > parent(s): 28f441e36aaaec15ce7d447ef709fad7fbc7cf7d > committer: Alexey Botchkov > timestamp: 2016-09-28 14:48:54 +0400 > message: > > MDEV-8320 Allow index usage for DATE(datetime_column) = const. > > Test for 'sargable functions' added. > First, t/range.test crashes after I apply the patch. MTR output is here: https://gist.github.com/spetrunia/d10165820664e0d18d4a667d44d226ee but I've got the crash on two different machines so should be easy to repeat > diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h > index 6d432bd..516bb07 100644 > --- a/sql/item_cmpfunc.h > +++ b/sql/item_cmpfunc.h > @@ -136,6 +136,14 @@ class Item_bool_func :public Item_int_func > { > protected: > /* > + Some functions modify it's arguments for the optimizer. > + So for example the condition 'Func(fieldX) = constY' turned into > + 'fieldX = cnuR(constY)' so that optimizer can use an index on fieldX. > + */ What's cnuR? Ok, I eventually got it, but the comments should not have such puzzles. > + Item *opt_args[3]; > + uint opt_arg_count; > + > + /* > +static Item_field *get_local_field (Item *field) > +{ > + Item *ri= field->real_item(); > + return (ri->type() == Item::FIELD_ITEM > + && !(field->used_tables() & OUTER_REF_TABLE_BIT) > + && !((Item_field *)ri)->get_depended_from()) ? (Item_field *) ri : 0; > +} Please fix indentation and add comments. Does this function do what is_local_field does, or there is some difference? > + > + > +static Item_field *field_in_sargable_func(Item *fn) > +{ > + fn= fn->real_item(); > + > + if (fn->type() == Item::FUNC_ITEM && > + strcmp(((Item_func *)fn)->func_name(), "cast_as_date") == 0) > + > + { > + Item_date_typecast *dt= (Item_date_typecast *) fn; > + return get_local_field(dt->arguments()[0]); > + } > + return 0; Please use NULL instead of 0, and !strcmp() instead of strcmp()=0. > @@ -5036,6 +5060,25 @@ Item_func_like::add_key_fields(JOIN *join, KEY_FIELD > **key_fields, > } > > > +bool Item_bool_rowready_func2::add_extra_key_fields(THD *thd, > + JOIN *join, KEY_FIELD > **key_fields, > + uint *and_level, > + table_map usable_tables, > + SARGABLE_PARAM **sargables) > +{ > + Item_field *f; > + if ((f= field_in_sargable_func(args[0])) && args[1]->const_item()) What is the difference between add_key_fields and add_extra_key_fields? Any cases where one should call one but not the other? Please also do indentation as coding style specifies. > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 41dc967..3124444 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -2569,6 +2569,39 @@ bool Item_date_typecast::get_date(MYSQL_TIME *ltime, > ulonglong fuzzy_date) > } > > > +bool Item_date_typecast::create_reverse_func(enum Functype cmp_type, > + THD *thd, Item *r_arg, uint *a_cnt, Item** > a) > +{ We need a specification of what exactly this function does, and a usage scenario in the comment. This function actually creates multiple (up to 3?) functions. If one has a condition DATE(t1.d) < '2000-01-04' then we get (gdb) p ((Item*)cond)->opt_arg_count $37 = 3 (gdb) p dbug_print_item(((Item*)cond)->opt_args[0]) $38 = 0x555557083e20 <dbug_item_print_buf> "t1.d" (gdb) p dbug_print_item(((Item*)cond)->opt_args[1]) $39 = 0x555557083e20 <dbug_item_print_buf> "day_begin('2000-01-19')" (gdb) p dbug_print_item(((Item*)cond)->opt_args[2]) $40 = 0x555557083e20 <dbug_item_print_buf> "day_end('2000-01-19')" which makes sense, but the description is lacking. Probably the name "create_reverse_func" is not good, because 1. multiple functions are created and 2. neither of them is the reverse. I can't suggest a better name at the moment, though. Let's both think about how to make this code cleared for an uninformed reader. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
