Cleanup of Item_func_group_concat

mysql-test/r/func_gconcat.result

@@ -166,7 +166,7 @@ set group_concat_max_len = 1024;
 select group_concat(sum(a)) from t1 group by grp;
 ERROR HY000: Invalid use of group function
 select grp,group_concat(c order by 2) from t1 group by grp;
-ERROR 42S22: Unknown column '2' in 'group statement'
+ERROR 42S22: Unknown column '2' in 'order clause'
 drop table t1;
 create table t1 ( URL_ID int(11), URL varchar(80));
 create table t2 ( REQ_ID int(11), URL_ID int(11));

sql/item_sum.cc

@@ -2575,8 +2575,9 @@ int group_concat_key_cmp_with_distinct(void* arg, byte* key1,
 				       byte* key2)
 {
   Item_func_group_concat* grp_item= (Item_func_group_concat*)arg;
+  TABLE *table= grp_item->table;
   Item **field_item, **end;
-  char *record= (char*) grp_item->table->record[0];
+  char *record= (char*) table->record[0] + table->s->null_bytes;
 
   for (field_item= grp_item->args, end= field_item + grp_item->arg_count_field;
        field_item < end;
@@ -2609,7 +2610,8 @@ int group_concat_key_cmp_with_order(void* arg, byte* key1, byte* key2)
 {
   Item_func_group_concat* grp_item= (Item_func_group_concat*) arg;
   ORDER **order_item, **end;
-  char *record= (char*) grp_item->table->record[0];
+  TABLE *table= grp_item->table;
+  char *record= (char*) table->record[0] + table->s->null_bytes;
 
   for (order_item= grp_item->order, end=order_item+ grp_item->arg_count_order;
        order_item < end;
@@ -2622,6 +2624,7 @@ int group_concat_key_cmp_with_order(void* arg, byte* key1, byte* key2)
       the temporary table, not the original field
     */
     Field *field= item->get_tmp_table_field();
+    /* If the item is a constant, there is no tmp table field */
     if (field)
     {
       int res;
@@ -2633,7 +2636,7 @@ int group_concat_key_cmp_with_order(void* arg, byte* key1, byte* key2)
   /*
     We can't return 0 because in that case the tree class would remove this
     item as double value. This would cause problems for case-changes and
-    if the the returned values are not the same we do the sort on.
+    if the returned values are not the same we do the sort on.
   */
   return 1;
 }
@@ -2665,48 +2668,48 @@ int group_concat_key_cmp_with_distinct_and_order(void* arg,byte* key1,
 int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
                   Item_func_group_concat *item)
 {
-  char buff[MAX_FIELD_WIDTH];
-  String tmp((char *)&buff,sizeof(buff),default_charset_info), tmp2;
-  char *record= (char*) item->table->record[0];
+  TABLE *table= item->table;
+  char *record= (char*) table->record[0] + table->s->null_bytes;
+  String tmp(table->record[1], table->s->reclength, default_charset_info), tmp2;
+  String &result= item->result;
+  Item **arg= item->args, **arg_end= item->args + item->arg_count_field;
 
-  if (item->result.length())
-    item->result.append(*item->separator);
+  if (result.length())
+    result.append(*item->separator);
 
   tmp.length(0);
 
-  for (uint i= 0; i < item->arg_count_field; i++)
+  for (; arg < arg_end; arg++)
   {
-    Item *show_item= item->args[i];
-    if (!show_item->const_item())
+    String *res;
+    if (! (*arg)->const_item())
     {
       /*
 	We have to use get_tmp_table_field() instead of
 	real_item()->get_tmp_table_field() because we want the field in
 	the temporary table, not the original field
+        We also can't use table->field array to access the fields
+        because it contains both order and arg list fields.
       */
-      Field *field= show_item->get_tmp_table_field();
-      String *res;
+      Field *field= (*arg)->get_tmp_table_field();
       char *save_ptr= field->ptr;
       uint offset= (uint) (save_ptr - record);
-      DBUG_ASSERT(offset < item->table->s->reclength);
+      DBUG_ASSERT(offset < table->s->reclength);
       field->ptr= (char *) key + offset;
       res= field->val_str(&tmp,&tmp2);
-      item->result.append(*res);
       field->ptr= save_ptr;
     }
     else
-    {
-      String *res= show_item->val_str(&tmp);
+      res= (*arg)->val_str(&tmp);
     if (res)
-        item->result.append(*res);
-    }
+      result.append(*res);
   }
 
-  /* stop if length of result more than group_concat_max_len */  
-  if (item->result.length() > item->group_concat_max_len)
+  /* stop if length of result more than max_length */
+  if (result.length() > item->max_length)
   {
     item->count_cut_values++;
-    item->result.length(item->group_concat_max_len);
+    result.length(item->max_length);
     item->warning_for_row= TRUE;
     return 1;
   }
@@ -2716,34 +2719,29 @@ int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
 
 /*
   Constructor of Item_func_group_concat
-  is_distinct - distinct
-  is_select - list of expression for show values
-  is_order - list of sort columns 
-  is_separator - string value of separator
+  distinct_arg - distinct
+  select_list - list of expression for show values
+  order_list - list of sort columns
+  separator_arg - string value of separator
 */
 
-Item_func_group_concat::Item_func_group_concat(bool is_distinct,
-					       List<Item> *is_select,
-					       SQL_LIST *is_order,
-					       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),
-   separator(is_separator), tree(&tree_base), table(0),
+Item_func_group_concat::
+Item_func_group_concat(bool distinct_arg, List<Item> *select_list,
+                       SQL_LIST *order_list, String *separator_arg)
+  :tmp_table_param(0), warning(0),
+   separator(separator_arg), tree(0), table(0),
    order(0), tables_list(0),
-   arg_count_order(0), arg_count_field(0),
-   count_cut_values(0)
+   arg_count_order(order_list ? order_list->elements : 0),
+   arg_count_field(select_list->elements),
+   count_cut_values(0),
+   distinct(distinct_arg),
+   warning_for_row(FALSE),
+   original(0)
 {
   Item *item_select;
   Item **arg_ptr;
 
-  original= 0;
-  quick_group= 0;
-  mark_as_sum_func();
-  order= 0;
-  group_concat_max_len= current_thd->variables.group_concat_max_len;
-    
-  arg_count_field= is_select->elements;
-  arg_count_order= is_order ? is_order->elements : 0;
+  quick_group= FALSE;
   arg_count= arg_count_field + arg_count_order;
 
   /*
@@ -2759,7 +2757,7 @@ Item_func_group_concat::Item_func_group_concat(bool is_distinct,
   order= (ORDER**)(args + arg_count);
 
   /* fill args items of show and sort */
-  List_iterator_fast<Item> li(*is_select);
+  List_iterator_fast<Item> li(*select_list);
 
   for (arg_ptr=args ; (item_select= li++) ; arg_ptr++)
     *arg_ptr= item_select;
@@ -2767,7 +2765,7 @@ Item_func_group_concat::Item_func_group_concat(bool is_distinct,
   if (arg_count_order)
   {
     ORDER **order_ptr= order;
-    for (ORDER *order_item= (ORDER*) is_order->first;
+    for (ORDER *order_item= (ORDER*) order_list->first;
          order_item != NULL;
          order_item= order_item->next)
     {
@@ -2781,24 +2779,20 @@ Item_func_group_concat::Item_func_group_concat(bool is_distinct,
 
 Item_func_group_concat::Item_func_group_concat(THD *thd,
                                                Item_func_group_concat *item)
-  :Item_sum(thd, item),item_thd(thd),
+  :Item_sum(thd, item),
   tmp_table_param(item->tmp_table_param),
-  max_elements_in_tree(item->max_elements_in_tree),
   warning(item->warning),
-  key_length(item->key_length), 
-  tree_mode(item->tree_mode),
-  distinct(item->distinct),
-  warning_for_row(item->warning_for_row),
   separator(item->separator),
   tree(item->tree),
   table(item->table),
   order(item->order),
   tables_list(item->tables_list),
-  group_concat_max_len(item->group_concat_max_len),
   arg_count_order(item->arg_count_order),
   arg_count_field(item->arg_count_field),
-  field_list_offset(item->field_list_offset),
   count_cut_values(item->count_cut_values),
+  distinct(item->distinct),
+  warning_for_row(item->warning_for_row),
+  always_null(item->always_null),
   original(item)
 {
   quick_group= item->quick_group;
@@ -2817,18 +2811,17 @@ void Item_func_group_concat::cleanup()
   */
   if (!original)
   {
-    THD *thd= current_thd;
+    delete tmp_table_param;
+    tmp_table_param= 0;
     if (table)
     {
+      THD *thd= table->in_use;
       free_tmp_table(thd, table);
       table= 0;
-    }
-    delete tmp_table_param;
-    tmp_table_param= 0;
-    if (tree_mode)
+      if (tree)
       {
-      tree_mode= 0;
         delete_tree(tree);
+        tree= 0;
       }
       if (warning)
       {
@@ -2838,15 +2831,13 @@ void Item_func_group_concat::cleanup()
         warning= 0;
       }
     }
+    DBUG_ASSERT(tree == 0);
+    DBUG_ASSERT(warning == 0);
+  }
   DBUG_VOID_RETURN;
 }
 
 
-Item_func_group_concat::~Item_func_group_concat()
-{
-}
-
-
 Item *Item_func_group_concat::copy_or_same(THD* thd)
 {
   return new (thd->mem_root) Item_func_group_concat(thd, this);
@@ -2859,14 +2850,9 @@ void Item_func_group_concat::clear()
   result.copy();
   null_value= TRUE;
   warning_for_row= FALSE;
-  if (table)
-  {
-    table->file->extra(HA_EXTRA_NO_CACHE);
-    table->file->delete_all_rows();
-    table->file->extra(HA_EXTRA_WRITE_CACHE);
-  }
-  if (tree_mode)
+  if (tree)
     reset_tree(tree);
+  /* No need to reset the table as we never call write_row */
 }
 
 
@@ -2895,29 +2881,23 @@ bool Item_func_group_concat::add()
   null_value= FALSE;
 
   TREE_ELEMENT *el= 0;                          // Only for safety
-  if (tree_mode)
-    el= tree_insert(tree, table->record[0], 0, tree->custom_arg);
+  if (tree)
+    el= tree_insert(tree, table->record[0] + table->s->null_bytes, 0,
+                    tree->custom_arg);
   /*
     If the row is not a duplicate (el->count == 1)
     we can dump the row here in case of GROUP_CONCAT(DISTINCT...)
     instead of doing tree traverse later.
   */
-  if (result.length() <= group_concat_max_len && 
+  if (result.length() <= max_length &&
       !warning_for_row &&
-      (!tree_mode || (el->count == 1 && distinct && !arg_count_order)))
-    dump_leaf_key(table->record[0], 1, this);
+      (!tree || (el->count == 1 && distinct && !arg_count_order)))
+    dump_leaf_key(table->record[0] + table->s->null_bytes, 1, this);
 
   return 0;
 }
 
 
-void Item_func_group_concat::reset_field()
-{
-  if (tree_mode)
-    reset_tree(tree);
-}
-
-
 bool
 Item_func_group_concat::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
 {
@@ -2930,10 +2910,11 @@ Item_func_group_concat::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
                MYF(0));
     return TRUE;
   }
+  if (!args)                      /* allocation in constructor may fail */
+    return TRUE;
 
   thd->allow_sum_func= 0;
   maybe_null= 0;
-  item_thd= thd;
 
   /*
     Fix fields for select list and ORDER clause
@@ -2951,12 +2932,8 @@ Item_func_group_concat::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
 
   result_field= 0;
   null_value= 1;
-  max_length= group_concat_max_len;
   thd->allow_sum_func= 1;
-  if (!(tmp_table_param= new TMP_TABLE_PARAM))
-    return TRUE;
-  /* We'll convert all blobs to varchar fields in the temporary table */
-  tmp_table_param->convert_blob_length= group_concat_max_len;
+  max_length= thd->variables.group_concat_max_len;
   tables_list= tables;
   fixed= 1;
   return FALSE;
@@ -2967,52 +2944,54 @@ bool Item_func_group_concat::setup(THD *thd)
 {
   List<Item> list;
   SELECT_LEX *select_lex= thd->lex->current_select;
-  uint const_fields;
   qsort_cmp2 compare_key;
   DBUG_ENTER("Item_func_group_concat::setup");
 
-  if (select_lex->linkage == GLOBAL_OPTIONS_TYPE)
-    DBUG_RETURN(1);
-
   /*
-    push all not constant fields to list and create temp table
+    Currently setup() can be called twice. Please add
+    assertion here when this is fixed.
   */
-  const_fields= 0;
+  if (table || tree)
+    DBUG_RETURN(FALSE);
+
+  if (!(tmp_table_param= new TMP_TABLE_PARAM))
+    DBUG_RETURN(TRUE);
+
+  /* We'll convert all blobs to varchar fields in the temporary table */
+  tmp_table_param->convert_blob_length= max_length;
+  /* Push all not constant fields to the list and create a temp table */
   always_null= 0;
   for (uint i= 0; i < arg_count_field; i++)
   {
     Item *item= args[i];
     if (list.push_back(item))
-      DBUG_RETURN(1);
+      DBUG_RETURN(TRUE);
     if (item->const_item())
     {
-      const_fields++;
       (void) item->val_int();
       if (item->null_value)
+      {
         always_null= 1;
+        break;
+      }
     }
   }
   if (always_null)
-    DBUG_RETURN(0);
+    DBUG_RETURN(FALSE);
 
   List<Item> all_fields(list);
-  if (arg_count_order)
-  {
-    bool hidden_group_fields;
-    setup_group(thd, args, tables_list, list, all_fields, *order,
-                &hidden_group_fields);
-  }
-
-  count_field_types(tmp_table_param,all_fields,0);
-  if (table)
-  {
   /*
-      We come here when we are getting the result from a temporary table,
-      not the original tables used in the query
+    Try to find every ORDER expression in the list of GROUP_CONCAT
+    arguments. If an expression is not found, prepend it to
+    "all_fields". The resulting field list is used as input to create
+    tmp table columns.
   */
-    free_tmp_table(thd, table);
-    tmp_table_param->cleanup();
-  }
+  if (arg_count_order &&
+      setup_order(thd, args, tables_list, list, all_fields, *order))
+    DBUG_RETURN(TRUE);
+
+  count_field_types(tmp_table_param,all_fields,0);
+  DBUG_ASSERT(table == 0);
   /*
     We have to create a temporary table to get descriptions of fields
     (types, sizes and so on).
@@ -3023,26 +3002,25 @@ bool Item_func_group_concat::setup(THD *thd)
     We need to set set_sum_field in true for storing value of blob in buffer
     of a record instead of a pointer of one.
   */
-  if (!(table=create_tmp_table(thd, tmp_table_param, all_fields, 
+  if (!(table= create_tmp_table(thd, tmp_table_param, all_fields,
                                 (ORDER*) 0, 0, TRUE,
                                 select_lex->options | thd->options,
-			       HA_POS_ERROR,(char *) "")))
-    DBUG_RETURN(1);
+                                HA_POS_ERROR, (char*) "")))
+    DBUG_RETURN(TRUE);
   table->file->extra(HA_EXTRA_NO_ROWS);
   table->no_rows= 1;
 
-  key_length= table->s->reclength;
 
-  /* Offset to first result field in table */
-  field_list_offset= table->s->fields - (list.elements - const_fields);
-
-  if (tree_mode)
-    delete_tree(tree);
-
-  /* choose function of sort */  
-  tree_mode= distinct || arg_count_order; 
-  if (tree_mode)
+  if (distinct || arg_count_order)
   {
+    /*
+      Need sorting: init tree and choose a function to sort.
+      Don't reserve space for NULLs: if any of gconcat arguments is NULL,
+      the row is not added to the result.
+    */
+    uint tree_key_length= table->s->reclength - table->s->null_bytes;
+
+    tree= &tree_base;
     if (arg_count_order)
     {
       if (distinct)
@@ -3055,27 +3033,16 @@ bool Item_func_group_concat::setup(THD *thd)
       compare_key= (qsort_cmp2) group_concat_key_cmp_with_distinct;
     }
     /*
-      Create a tree of sort. Tree is used for a sort and a remove double 
-      values (according with syntax of the function). If function doesn't
-      contain DISTINCT and ORDER BY clauses, we don't create this tree.
+      Create a tree for sorting. The tree is used to sort and to remove
+      duplicate values (according to the syntax of this function). If there
+      is no DISTINCT or ORDER BY clauses, we don't create this tree.
     */
     init_tree(tree, min(thd->variables.max_heap_table_size,
                         thd->variables.sortbuff_size/16), 0,
-              key_length, compare_key, 0, NULL, (void*) this);
-    max_elements_in_tree= (key_length ? 
-			   thd->variables.max_heap_table_size/key_length : 1);
-  };
-
-  /*
-    Copy table and tree_mode if they belong to this item (if item have not 
-    pointer to original item from which was made copy => it own its objects)
-  */
-  if (original)
-  {
-    original->table= table;
-    original->tree_mode= tree_mode;
+              tree_key_length, compare_key, 0, NULL, (void*) this);
   }
-  DBUG_RETURN(0);
+
+  DBUG_RETURN(FALSE);
 }
 
 
@@ -3083,10 +3050,10 @@ bool Item_func_group_concat::setup(THD *thd)
 
 void Item_func_group_concat::make_unique()
 {
+  tmp_table_param= 0;
   table=0;
   original= 0;
-  tree_mode= 0; // to prevent delete_tree call on uninitialized tree
-  tree= &tree_base;
+  tree= 0;
 }
 
 
@@ -3096,16 +3063,17 @@ String* Item_func_group_concat::val_str(String* str)
   if (null_value)
     return 0;
   if (count_cut_values && !warning)
-    warning= push_warning(item_thd, MYSQL_ERROR::WARN_LEVEL_WARN,
+  {
+    DBUG_ASSERT(table);
+    warning= push_warning(table->in_use, MYSQL_ERROR::WARN_LEVEL_WARN,
                           ER_CUT_VALUE_GROUP_CONCAT,
                           ER(ER_CUT_VALUE_GROUP_CONCAT));
+  }
   if (result.length())
     return &result;
-  if (tree_mode)
-  {
+  if (tree)
     tree_walk(tree, (tree_walk_action)&dump_leaf_key, (void*)this,
               left_root_right);
-  }
   return &result;
 }
 

sql/item_sum.h

@@ -834,27 +834,8 @@ class MYSQL_ERROR;
 
 class Item_func_group_concat : public Item_sum
 {
-  THD *item_thd;
   TMP_TABLE_PARAM *tmp_table_param;
-  uint max_elements_in_tree;
   MYSQL_ERROR *warning;
-  uint key_length;
-  bool tree_mode;
-  bool distinct;
-  bool warning_for_row;
-  bool always_null;
-
-  friend int group_concat_key_cmp_with_distinct(void* arg, byte* key1,
-					      byte* key2);
-  friend int group_concat_key_cmp_with_order(void* arg, byte* key1,
-					     byte* key2);
-  friend int group_concat_key_cmp_with_distinct_and_order(void* arg,
-							byte* key1,
-							byte* key2);
-  friend int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
-			   Item_func_group_concat *group_concat_item);
-
- public:
   String result;
   String *separator;
   TREE tree_base;
@@ -862,22 +843,34 @@ class Item_func_group_concat : public Item_sum
   TABLE *table;
   ORDER **order;
   TABLE_LIST *tables_list;
-  ulong group_concat_max_len;
-  uint arg_count_order;
-  uint arg_count_field;
-  uint field_list_offset;
+  uint arg_count_order;               // total count of ORDER BY items
+  uint arg_count_field;               // count of arguments
   uint count_cut_values;
+  bool distinct;
+  bool warning_for_row;
+  bool always_null;
   /*
     Following is 0 normal object and pointer to original one for copy
     (to correctly free resources)
   */
   Item_func_group_concat *original;
 
+  friend int group_concat_key_cmp_with_distinct(void* arg, byte* key1,
+					      byte* key2);
+  friend int group_concat_key_cmp_with_order(void* arg, byte* key1,
+					     byte* key2);
+  friend int group_concat_key_cmp_with_distinct_and_order(void* arg,
+							byte* key1,
+							byte* key2);
+  friend int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
+			   Item_func_group_concat *group_concat_item);
+
+public:
   Item_func_group_concat(bool is_distinct,List<Item> *is_select,
                          SQL_LIST *is_order,String *is_separator);
 
   Item_func_group_concat(THD *thd, Item_func_group_concat *item);
-  ~Item_func_group_concat();
+  ~Item_func_group_concat() {}
   void cleanup();
 
   enum Sumfunctype sum_func () const {return GROUP_CONCAT_FUNC;}
@@ -885,11 +878,11 @@ class Item_func_group_concat : public Item_sum
   virtual Item_result result_type () const { return STRING_RESULT; }
   void clear();
   bool add();
-  void reset_field();
+  void reset_field() {}                         // not used
+  void update_field() {}                        // not used
   bool fix_fields(THD *, TABLE_LIST *, Item **);
   bool setup(THD *thd);
   void make_unique();
-  virtual void update_field() {}
   double val_real()
   {
     String *res;  res=val_str(&str_value);