[Maria-developers] MDEV-4119 patch comments
![](https://secure.gravatar.com/avatar/c7422c379fec4aa4d99a8a137bf10594.jpg?s=120&d=mm&r=g)
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
Hi Monty, Please find my comments on the patch for MDEV-4119 below. Sorry for the delay. 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
![](https://secure.gravatar.com/avatar/68cce0b6e372c2c66f4d66c6356102af.jpg?s=120&d=mm&r=g)
Hi!
On Mon, Jul 6, 2015 at 7:37 PM, Sergey Petrunia
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
participants (2)
-
Michael Widenius
-
Sergey Petrunia