maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Monty,

Please find my comments on the patch for MDEV-4119 below. Sorry for the delay.


> diff --git a/sql/item.cc b/sql/item.cc
> index b4de973..3b1f8b7 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -1705,69 +1705,98 @@ class Item_aggregate_ref : public Item_ref
>  
>    @param thd			Thread handler
>    @param ref_pointer_array	Pointer to array of reference fields
> -  @param fields		All fields in select
> +  @param fields		        All fields in select
>    @param ref			Pointer to item
> -  @param skip_registered       <=> function be must skipped for registered
> -                               SUM items
> +  @param split_flags            Zero or more of the following flags
> +	                        SPLIT_FUNC_SKIP_REGISTERED:
> +                                Function be must skipped for registered SUM
> +                                SUM items
> +                                SPLIT_FUNC_SELECT
> +                                We are called on the select level and have to
> +                                register items operated on sum function
Typo, it's not SPLIT_FUNC_SELECT, it is SPLIT_SUM_SELECT
>  
>    @note
> -    This is from split_sum_func2() for items that should be split
> -
>      All found SUM items are added FIRST in the fields list and
>      we replace the item with a reference.
>  
> +    If this is an item in the SELECT list then we also have to split out
> +    all arguments to functions used together with the sum function.
> +    For example in case of SELECT A*sum(B) we have to split out both
> +    A and sum(B).
> +    This is not needed for ORDER BY, GROUP BY or HAVING as all references
> +    to items in the select list are already of type REF
> +
>      thd->fatal_error() may be called if we are out of memory
>  */
>  
>  void Item::split_sum_func2(THD *thd, Item **ref_pointer_array,
>                             List<Item> &fields, Item **ref, 
> -                           bool skip_registered)
> +                           uint split_flags)
>  {
> -  /* An item of type Item_sum  is registered <=> ref_by != 0 */ 
> -  if (type() == SUM_FUNC_ITEM && skip_registered && 
> -      ((Item_sum *) this)->ref_by)
> -    return;
> -  if ((type() != SUM_FUNC_ITEM && with_sum_func) ||
> -      (type() == FUNC_ITEM &&
> -       (((Item_func *) this)->functype() == Item_func::ISNOTNULLTEST_FUNC ||
> -        ((Item_func *) this)->functype() == Item_func::TRIG_COND_FUNC)))
> +  if (unlikely(type() == SUM_FUNC_ITEM))
>    {
> -    /* Will split complicated items and ignore simple ones */
> -    split_sum_func(thd, ref_pointer_array, fields);
> +    /* An item of type Item_sum is registered if ref_by != 0 */
> +    if ((split_flags & SPLIT_SUM_SKIP_REGISTERED) && 
> +        ((Item_sum *) this)->ref_by)
> +      return;
>    }
> -  else if ((type() == SUM_FUNC_ITEM || (used_tables() & ~PARAM_TABLE_BIT)) &&
> -           type() != SUBSELECT_ITEM &&
> -           (type() != REF_ITEM ||
> -           ((Item_ref*)this)->ref_type() == Item_ref::VIEW_REF))
> +  else
>    {
> -    /*
> -      Replace item with a reference so that we can easily calculate
> -      it (in case of sum functions) or copy it (in case of fields)
> -
> -      The test above is to ensure we don't do a reference for things
> -      that are constants (PARAM_TABLE_BIT is in effect a constant)
> -      or already referenced (for example an item in HAVING)
> -      Exception is Item_direct_view_ref which we need to convert to
> -      Item_ref to allow fields from view being stored in tmp table.
> -    */
> -    Item_aggregate_ref *item_ref;
> -    uint el= fields.elements;
> -    /*
> -      If this is an item_ref, get the original item
> -      This is a safety measure if this is called for things that is
> -      already a reference.
> -    */
> -    Item *real_itm= real_item();
> +    /* Not a SUM() function */
> +    if (unlikely((!with_sum_func && !(split_flags & SPLIT_SUM_SELECT))))
> +    {
> +      /*
> +        This is not a SUM function and there are no SUM functions inside.
> +        Nothing more to do.
> +      */
> +      return;
> +    }
> +    if (likely(with_sum_func ||
> +               (type() == FUNC_ITEM &&
> +                (((Item_func *) this)->functype() ==
> +                 Item_func::ISNOTNULLTEST_FUNC ||
> +                 ((Item_func *) this)->functype() ==
> +                 Item_func::TRIG_COND_FUNC))))
> +    {
> +      /* Will call split_sum_func2() for all items */
> +      split_sum_func(thd, ref_pointer_array, fields, split_flags);
> +      return;
> +    }
>  
> -    ref_pointer_array[el]= real_itm;
> -    if (!(item_ref= new Item_aggregate_ref(&thd->lex->current_select->context,
> -                                           ref_pointer_array + el, 0, name)))
> -      return;                                   // fatal_error is set
> -    if (type() == SUM_FUNC_ITEM)
> -      item_ref->depended_from= ((Item_sum *) this)->depended_from(); 
> -    fields.push_front(real_itm);
> -    thd->change_item_tree(ref, item_ref);
> +    if (unlikely((!(used_tables() & ~PARAM_TABLE_BIT) ||
> +                  type() == SUBSELECT_ITEM ||
> +                  (type() == REF_ITEM &&
> +                   ((Item_ref*)this)->ref_type() != Item_ref::VIEW_REF))))
> +        return;
>    }
> +
> +  /*
> +    Replace item with a reference so that we can easily calculate
> +    it (in case of sum functions) or copy it (in case of fields)
> +
> +    The test above is to ensure we don't do a reference for things
> +    that are constants (PARAM_TABLE_BIT is in effect a constant)
> +    or already referenced (for example an item in HAVING)
> +    Exception is Item_direct_view_ref which we need to convert to
> +    Item_ref to allow fields from view being stored in tmp table.
> +  */
> +  Item_aggregate_ref *item_ref;
> +  uint el= fields.elements;
> +  /*
> +    If this is an item_ref, get the original item
> +    This is a safety measure if this is called for things that is
> +    already a reference.
> +  */
> +  Item *real_itm= real_item();
> +
> +  ref_pointer_array[el]= real_itm;
> +  if (!(item_ref= new Item_aggregate_ref(&thd->lex->current_select->context,
> +                                         ref_pointer_array + el, 0, name)))
> +    return;                                   // fatal_error is set
> +  if (type() == SUM_FUNC_ITEM)
> +    item_ref->depended_from= ((Item_sum *) this)->depended_from(); 
> +  fields.push_front(real_itm);
> +  thd->change_item_tree(ref, item_ref);
>  }
>  
>  
> diff --git a/sql/item.h b/sql/item.h
> index 8254359..44649bf 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -74,6 +74,10 @@ char_to_byte_length_safe(uint32 char_length_arg, uint32 mbmaxlen_arg)
>  }
>  
>  
> +/* Bits for the split_sum_func() function */
> +#define SPLIT_SUM_SKIP_REGISTERED 1     /* Skip registered funcs */
> +#define SPLIT_SUM_SELECT 2		/* SELECT item; Split all parts */
> +
>  /*
>     "Declared Type Collation"
>     A combination of collation and its derivation.
> @@ -1169,10 +1173,10 @@ class Item: public Type_std_attributes
>      return false;
>    }
>    virtual void split_sum_func(THD *thd, Item **ref_pointer_array,
> -                              List<Item> &fields) {}
> +                              List<Item> &fields, uint flags) {}
>    /* Called for items that really have to be split */
>    void split_sum_func2(THD *thd, Item **ref_pointer_array, List<Item> &fields,
> -                       Item **ref, bool skip_registered);
> +                       Item **ref, uint flags);
>    virtual bool get_date(MYSQL_TIME *ltime, ulonglong fuzzydate);
>    bool get_time(MYSQL_TIME *ltime)
>    { return get_date(ltime, TIME_TIME_ONLY | TIME_INVALID_DATES); }
> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index 7b34e64..caa645d 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -4575,12 +4575,13 @@ void Item_cond::traverse_cond(Cond_traverser traverser,
>  */
>  
>  void Item_cond::split_sum_func(THD *thd, Item **ref_pointer_array,
> -                               List<Item> &fields)
> +                               List<Item> &fields, uint flags)
>  {
>    List_iterator<Item> li(list);
>    Item *item;
>    while ((item= li++))
> -    item->split_sum_func2(thd, ref_pointer_array, fields, li.ref(), TRUE);
> +    item->split_sum_func2(thd, ref_pointer_array, fields, li.ref(),
> +                          flags | SPLIT_SUM_SKIP_REGISTERED);
Why not clear SPLIT_SUM_SELECT flag here? 
The Item_cond may be at the select list, but its parts are definitely not in
the select list?

The same applies to Item_row::split_sum_func and Item_func::split_sum_func.

>  }
>  
>  
> diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
> index 7d89cf5..ceab597 100644
> --- a/sql/item_cmpfunc.h
> +++ b/sql/item_cmpfunc.h
> @@ -1759,7 +1759,8 @@ class Item_cond :public Item_bool_func
>                        SARGABLE_PARAM **sargables);
>    SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr);
>    virtual void print(String *str, enum_query_type query_type);
> -  void split_sum_func(THD *thd, Item **ref_pointer_array, List<Item> &fields);
> +  void split_sum_func(THD *thd, Item **ref_pointer_array, List<Item> &fields,
> +                      uint flags);
>    friend int setup_conds(THD *thd, TABLE_LIST *tables, TABLE_LIST *leaves,
>                           COND **conds);
>    void top_level_item() { abort_on_null=1; }
> diff --git a/sql/item_func.cc b/sql/item_func.cc
> index 3e78be8..684724b 100644
> --- a/sql/item_func.cc
> +++ b/sql/item_func.cc
> @@ -419,11 +419,12 @@ Item *Item_func::compile(Item_analyzer analyzer, uchar **arg_p,
>  */
>  
>  void Item_func::split_sum_func(THD *thd, Item **ref_pointer_array,
> -                               List<Item> &fields)
> +                               List<Item> &fields, uint flags)
>  {
>    Item **arg, **arg_end;
>    for (arg= args, arg_end= args+arg_count; arg != arg_end ; arg++)
> -    (*arg)->split_sum_func2(thd, ref_pointer_array, fields, arg, TRUE);
> +    (*arg)->split_sum_func2(thd, ref_pointer_array, fields, arg,
> +                            flags | SPLIT_SUM_SKIP_REGISTERED);
Why not clear SPLIT_SUM_SELECT flag here? (same question as in Item_cond::split_sum_func)
>  }
>  
>  
........
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index 4a50003..ce02484 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -1944,8 +1954,9 @@ int JOIN::init_execution()
>    /*
>      Enable LIMIT ROWS EXAMINED during query execution if:
>      (1) This JOIN is the outermost query (not a subquery or derived table)
> -        This ensures that the limit is enabled when actual execution begins, and
> -        not if a subquery is evaluated during optimization of the outer query.
> +        This ensures that the limit is enabled when actual execution begins,
> +        and not if a subquery is evaluated during optimization of the outer
> +        query.
>      (2) This JOIN is not the result of a UNION. In this case do not apply the
>          limit in order to produce the partial query result stored in the
>          UNION temp table.
> @@ -2562,8 +2573,12 @@ void JOIN::exec_inner()
>        skip_sort_order= 0;
>      }
>      bool made_call= false;
> +    SQL_SELECT *select= join_tab[const_tables].select;
This line causes a crash in a few tests:

main.select_pkeycache main.select main.select_jcl6 main.subselect3 main.subselect3_jcl6

The reason is that execution gets here when the join is degenerate and
join_tab==NULL.

>      if (order && 
> -        (order != group_list || !(select_options & SELECT_BIG_RESULT)) &&
> +        (order != group_list || !(select_options & SELECT_BIG_RESULT) ||
> +         (select && select->quick &&
> +          select->quick->get_type() == QUICK_SELECT_I::QS_TYPE_GROUP_MIN_MAX))
> +        &&
>  	(const_tables == table_count ||
>   	 ((simple_order || skip_sort_order) &&
>  	  (made_call=true) &&

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog