From b25315469e118c89c964d1c266e7164d3580eda5 Mon Sep 17 00:00:00 2001 From: unknown Date: Wed, 29 Mar 2006 23:30:34 +0400 Subject: [PATCH] Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries The GROUP_CONCAT uses its own temporary table. When ROLLUP is present it creates the second copy of Item_func_group_concat. This copy receives the same list of arguments that original group_concat does. When the copy is set up the result_fields of functions from the argument list are reset to the temporary table of this copy. As a result of this action data from functions flow directly to the ROLLUP copy and the original group_concat functions shows wrong result. Since queries with COUNT(DISTINCT ...) use temporary tables to store the results the COUNT function they are also affected by this bug. The idea of the fix is to copy content of the result_field for the function under GROUP_CONCAT/COUNT from the first temporary table to the second one, rather than setting result_field to point to the second temporary table. To achieve this goal force_copy_fields flag is added to Item_func_group_concat and Item_sum_count_distinct classes. This flag is initialized to 0 and set to 1 into the make_unique() member function of both classes. To the TMP_TABLE_PARAM structure is modified to include the similar flag as well. The create_tmp_table() function passes that flag to create_tmp_field(). When the flag is set the create_tmp_field() function will set result_field as a source field and will not reset that result field to newly created field for Item_func_result_field and its descendants. Due to this there will be created copy func to copy data from old result_field to newly created field. mysql-test/t/func_gconcat.test: Added test for bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries mysql-test/r/func_gconcat.result: Added test for bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries sql/sql_table.cc: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added 0 as a last parameter to create_tmp_field() to force old behaviour. sql/sql_select.cc: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added the flag 'make_copy_field' to create_tmp_field(), so that for Item_result_field descendants create_tmp_field() sets the item's result field as a source field and deny resetting that result field to a new value. sql/sql_class.h: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added the flag 'force_copy_fields' to the structure TMP_TABLE_PARAM in order to make create_tmp_field() force the creation of 'copy_field' objects. sql/mysql_priv.h: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added the bool parameter 'make_copy_field' to create_tmp_field(). sql/item_sum.cc: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added initialization of the force_copy_fields flag and passing it to create_tmp_table() through TMP_TBLE_PARAM in the Item_func_group_concat and Item_sum_count_distinct member functions. sql/item_sum.h: Fixed bug#15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries Added the flag 'force_copy_fields' to the Item_func_group_concat and Item_sum_count_distinct classes. --- mysql-test/r/func_gconcat.result | 15 +++++++++++++++ mysql-test/t/func_gconcat.test | 8 ++++++++ sql/item_sum.cc | 6 ++++++ sql/item_sum.h | 9 ++++++--- sql/mysql_priv.h | 5 +++-- sql/sql_class.h | 5 +++-- sql/sql_select.cc | 21 +++++++++++++++------ sql/sql_table.cc | 2 +- 8 files changed, 57 insertions(+), 14 deletions(-) diff --git a/mysql-test/r/func_gconcat.result b/mysql-test/r/func_gconcat.result index 200b4a4d53b..3cb8da7f63f 100644 --- a/mysql-test/r/func_gconcat.result +++ b/mysql-test/r/func_gconcat.result @@ -589,3 +589,18 @@ GROUP_CONCAT(a ORDER BY a) ,x ,z DROP TABLE t1; +create table t1(f1 int); +insert into t1 values(1),(2),(3); +select f1, group_concat(f1+1) from t1 group by f1 with rollup; +f1 group_concat(f1+1) +1 2 +2 3 +3 4 +NULL 2,3,4 +select count(distinct (f1+1)) from t1 group by f1 with rollup; +count(distinct (f1+1)) +1 +1 +1 +3 +drop table t1; diff --git a/mysql-test/t/func_gconcat.test b/mysql-test/t/func_gconcat.test index eb1cbc3d667..f116aa6f164 100644 --- a/mysql-test/t/func_gconcat.test +++ b/mysql-test/t/func_gconcat.test @@ -382,4 +382,12 @@ SELECT GROUP_CONCAT(a ORDER BY a) FROM t1 GROUP BY id; DROP TABLE t1; +# +# Bug #15560: GROUP_CONCAT wasn't ready for WITH ROLLUP queries +# +create table t1(f1 int); +insert into t1 values(1),(2),(3); +select f1, group_concat(f1+1) from t1 group by f1 with rollup; +select count(distinct (f1+1)) from t1 group by f1 with rollup; +drop table t1; # End of 4.1 tests diff --git a/sql/item_sum.cc b/sql/item_sum.cc index 43468adea1a..4b522cf06fa 100644 --- a/sql/item_sum.cc +++ b/sql/item_sum.cc @@ -1185,6 +1185,7 @@ void Item_sum_count_distinct::make_unique() original= 0; use_tree= 0; // to prevent delete_tree call on uninitialized tree tree= &tree_base; + force_copy_fields= 1; } @@ -1219,6 +1220,7 @@ bool Item_sum_count_distinct::setup(THD *thd) free_tmp_table(thd, table); tmp_table_param->cleanup(); } + tmp_table_param->force_copy_fields= force_copy_fields; if (!(table= create_tmp_table(thd, tmp_table_param, list, (ORDER*) 0, 1, 0, select_lex->options | thd->options, @@ -1724,6 +1726,7 @@ Item_func_group_concat::Item_func_group_concat(bool is_distinct, String *is_separator) :Item_sum(), tmp_table_param(0), max_elements_in_tree(0), warning(0), key_length(0), tree_mode(0), distinct(is_distinct), warning_for_row(0), + force_copy_fields(0), separator(is_separator), tree(&tree_base), table(0), order(0), tables_list(0), arg_count_order(0), arg_count_field(0), @@ -1785,6 +1788,7 @@ Item_func_group_concat::Item_func_group_concat(THD *thd, tree_mode(item->tree_mode), distinct(item->distinct), warning_for_row(item->warning_for_row), + force_copy_fields(item->force_copy_fields), separator(item->separator), tree(item->tree), table(item->table), @@ -2004,6 +2008,7 @@ bool Item_func_group_concat::setup(THD *thd) free_tmp_table(thd, table); tmp_table_param->cleanup(); } + tmp_table_param->force_copy_fields= force_copy_fields; /* We have to create a temporary table to get descriptions of fields (types, sizes and so on). @@ -2079,6 +2084,7 @@ void Item_func_group_concat::make_unique() original= 0; tree_mode= 0; // to prevent delete_tree call on uninitialized tree tree= &tree_base; + force_copy_fields= 1; } diff --git a/sql/item_sum.h b/sql/item_sum.h index d53d8d861ae..c972240fcba 100644 --- a/sql/item_sum.h +++ b/sql/item_sum.h @@ -185,6 +185,7 @@ class Item_sum_count_distinct :public Item_sum_int TMP_TABLE_PARAM *tmp_table_param; TREE tree_base; TREE *tree; + bool force_copy_fields; /* Following is 0 normal object and pointer to original one for copy (to correctly free resources) @@ -226,15 +227,16 @@ class Item_sum_count_distinct :public Item_sum_int public: Item_sum_count_distinct(List &list) :Item_sum_int(list), table(0), used_table_cache(~(table_map) 0), - tmp_table_param(0), tree(&tree_base), original(0), use_tree(0), - always_null(0) + tmp_table_param(0), tree(&tree_base), force_copy_fields(0), original(0), + use_tree(0), always_null(0) { quick_group= 0; } Item_sum_count_distinct(THD *thd, Item_sum_count_distinct *item) :Item_sum_int(thd, item), table(item->table), used_table_cache(item->used_table_cache), field_lengths(item->field_lengths), tmp_table_param(item->tmp_table_param), - tree(item->tree), original(item), key_length(item->key_length), + tree(item->tree), force_copy_fields(item->force_copy_fields), + original(item), key_length(item->key_length), max_elements_in_tree(item->max_elements_in_tree), rec_offset(item->rec_offset), use_tree(item->use_tree), always_null(item->always_null) @@ -685,6 +687,7 @@ class Item_func_group_concat : public Item_sum bool distinct; bool warning_for_row; bool always_null; + bool force_copy_fields; friend int group_concat_key_cmp_with_distinct(void* arg, byte* key1, byte* key2); diff --git a/sql/mysql_priv.h b/sql/mysql_priv.h index 429a71b4437..72424819078 100644 --- a/sql/mysql_priv.h +++ b/sql/mysql_priv.h @@ -537,8 +537,9 @@ int mysql_union(THD *thd, LEX *lex, select_result *result, SELECT_LEX_UNIT *unit); int mysql_handle_derived(LEX *lex); Field *create_tmp_field(THD *thd, TABLE *table,Item *item, Item::Type type, - Item ***copy_func, Field **from_field, - bool group, bool modify_item, uint convert_blob_length); + Item ***copy_func, Field **from_field, + bool group, bool modify_item, uint convert_blob_length, + bool make_copy_field); int mysql_prepare_table(THD *thd, HA_CREATE_INFO *create_info, List &fields, List &keys, uint &db_options, diff --git a/sql/sql_class.h b/sql/sql_class.h index 41170192892..1bd7dcac03c 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1334,10 +1334,11 @@ public: bool using_indirect_summary_function; /* If >0 convert all blob fields to varchar(convert_blob_length) */ uint convert_blob_length; - + bool force_copy_fields; TMP_TABLE_PARAM() :copy_field(0), group_parts(0), - group_length(0), group_null_parts(0), convert_blob_length(0) + group_length(0), group_null_parts(0), convert_blob_length(0), + force_copy_fields(0) {} ~TMP_TABLE_PARAM() { diff --git a/sql/sql_select.cc b/sql/sql_select.cc index ec62e80ba13..d7aa617cffd 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -4984,7 +4984,8 @@ static Field* create_tmp_field_from_item(THD *thd, Item *item, TABLE *table, Field *create_tmp_field(THD *thd, TABLE *table,Item *item, Item::Type type, Item ***copy_func, Field **from_field, - bool group, bool modify_item, uint convert_blob_length) + bool group, bool modify_item, uint convert_blob_length, + bool make_copy_field) { switch (type) { case Item::SUM_FUNC_ITEM: @@ -5071,7 +5072,13 @@ Field *create_tmp_field(THD *thd, TABLE *table,Item *item, Item::Type type, case Item::REF_ITEM: case Item::NULL_ITEM: case Item::VARBIN_ITEM: - return create_tmp_field_from_item(thd, item, table, copy_func, modify_item, + if (make_copy_field) + { + DBUG_ASSERT(((Item_result_field*)item)->result_field); + *from_field= ((Item_result_field*)item)->result_field; + } + return create_tmp_field_from_item(thd, item, table, (make_copy_field ? 0 : + copy_func), modify_item, convert_blob_length); case Item::TYPE_HOLDER: return ((Item_type_holder *)item)->make_field_by_type(table); @@ -5110,7 +5117,7 @@ create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List &fields, Item **copy_func; MI_COLUMNDEF *recinfo; uint temp_pool_slot=MY_BIT_NONE; - + bool force_copy_fields= param->force_copy_fields; DBUG_ENTER("create_tmp_table"); DBUG_PRINT("enter",("distinct: %d save_sum_fields: %d rows_limit: %lu group: %d", (int) distinct, (int) save_sum_fields, @@ -5241,7 +5248,7 @@ create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List &fields, Field *new_field= create_tmp_field(thd, table, arg, arg->type(), ©_func, tmp_from_field, group != 0,not_all_columns, - param->convert_blob_length); + param->convert_blob_length, 0); if (!new_field) goto err; // Should be OOM tmp_from_field++; @@ -5279,8 +5286,10 @@ create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List &fields, */ Field *new_field= create_tmp_field(thd, table, item, type, ©_func, tmp_from_field, group != 0, - not_all_columns || group !=0, - param->convert_blob_length); + !force_copy_fields && + (not_all_columns || group !=0), + param->convert_blob_length, + force_copy_fields); if (!new_field) { if (thd->is_fatal_error) diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 71cbc0be1e3..1cb49f9933c 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -1562,7 +1562,7 @@ TABLE *create_table_from_items(THD *thd, HA_CREATE_INFO *create_info, field=item->tmp_table_field(&tmp_table); else field=create_tmp_field(thd, &tmp_table, item, item->type(), - (Item ***) 0, &tmp_field, 0, 0, 0); + (Item ***) 0, &tmp_field, 0, 0, 0, 0); if (!field || !(cr_field=new create_field(field,(item->type() == Item::FIELD_ITEM ? ((Item_field *)item)->field :