maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

In-Reply-To: <20160928105123.6D948140DDC@nebo.localdomain>
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