maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Mon, Jul 6, 2015 at 7:37 PM, Sergey Petrunia <sergey@xxxxxxxxxxx> wrote:
> Hi Monty,
>
> Please find my comments on the patch for MDEV-4119 below. Sorry for the delay.

No problem, thanks for looking into this.

>> +  @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

Fixed

<cut>

>> +    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

I left the above as this was here to explain why we can not remove
SPLIT_FUNC_SELECT from any arguments.

<cut>

>>  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.

We can't ever clear SPLIT_SUM_SELECT, which is what I tried to explain
in the above comment.

I noticed this in the following query, part of group_by.test

select a*sum(b) from t1 where a=1 group by c;

The first select item is Item_mul(Item_field(a), Item_sum(Item_field(b)))

We can't remove SPLIT_SUM_SELECT for the arguments to Item_mul, as in this
case Item_field(a) would not be converted to an Item_ref.
The end result is that 'a' would be calculated from the last row found in
the table, not for the last row matching the where or part of the group.

Forcing 'a' to be a ref, will ensure that 'a' will be stored in the
created temporary and when we access it when calculating a*sum(b) it
will have the correct value for the group.

So the rule is:

- If we are in the SELECT, we have to change all references to field
to Item_ref, as long as they are not inside a SUM() function.
- For ORDER BY, HAVING, GROUP BY it's not needed as all fields are
  automaticly converted to references as part of fix_fields()

<cut>

>> +++ 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)

Same answer as above.

>> +++ 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:

Sorry about that; I had already fixed it shortly after I gave you the
original patch.  I fixed it by adding

if (order && join_tab)

Before the above row.

Regards,
Monty