bk commit into 5.0 tree (konstantin:1.1817)

[konstantin]

Below is the list of changes that have just been committed into a local
5.0 repository of kostja. When kostja does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.1817 05/03/17 03:03:04 konstantin@stripped +3 -0
  Cleanup for Item_func_group_concat (GROUP_CONCAT)

  sql/item_sum.h
    1.80 05/03/17 03:02:56 konstantin@stripped +20 -27
    Cleanup for Item_func_group_concat: remove unneded variables, methods,
    move a bunch of variables to private: section.

  sql/item_sum.cc
    1.130 05/03/17 03:02:56 konstantin@stripped +112 -142
    Cleanup of Item_func_group_concat:
    - last unobvious things commented
    - don't store NULLs in the tree.
    - remove unneeded variables.
    - use setup_order, not setup_group to setup group concat order list

  mysql-test/r/func_gconcat.result
    1.34 05/03/17 03:02:56 konstantin@stripped +1 -1
    Error message corrected.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	konstantin
# Host:	dragonfly.local
# Root:	/media/sda1/mysql/mysql-5.0-926

--- 1.129/sql/item_sum.cc	2005-03-16 17:10:55 +03:00
+++ 1.130/sql/item_sum.cc	2005-03-17 03:02:56 +03:00
@@ -2575,8 +2575,9 @@
 				       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 @@
 {
   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 @@
       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 @@
   /*
     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 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);
-      if (res)
-        item->result.append(*res);
-    }
+    else
+      res= (*arg)->val_str(&tmp);
+    if (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;
   }
@@ -2726,26 +2729,22 @@
 					       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),
+  :tmp_table_param(0), warning(0),
+   separator(is_separator), tree(0), table(0),
    order(0), tables_list(0),
-   arg_count_order(0), arg_count_field(0),
-   count_cut_values(0)
+   arg_count_order(is_order ? is_order->elements : 0),
+   arg_count_field(is_select->elements),
+   count_cut_values(0),
+   distinct(is_distinct),
+   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;
-  
+
   /*
     We need to allocate:
     args - arg_count_field+arg_count_order
@@ -2781,24 +2780,20 @@
 
 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,36 +2812,33 @@
   */
   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;
+      if (tree)
+      {
+        delete_tree(tree);
+        tree= 0;
+      }
+      if (warning)
+      {
+        char warn_buff[MYSQL_ERRMSG_SIZE];
+        sprintf(warn_buff, ER(ER_CUT_VALUE_GROUP_CONCAT), count_cut_values);
+        warning->set_msg(thd, warn_buff);
+        warning= 0;
+      }
     }
-    delete tmp_table_param;
-    tmp_table_param= 0;
-    if (tree_mode)
-    {
-      tree_mode= 0;
-      delete_tree(tree); 
-    }
-    if (warning)
-    {
-      char warn_buff[MYSQL_ERRMSG_SIZE];
-      sprintf(warn_buff, ER(ER_CUT_VALUE_GROUP_CONCAT), count_cut_values);
-      warning->set_msg(thd, warn_buff);
-      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 +2851,9 @@
   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 +2882,23 @@
   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 +2911,11 @@
                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 +2933,8 @@
 
   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,17 +2945,24 @@
 {
   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);
+  /*
+    Currently setup() can be called twice. Please add
+    assertion here when this is fixed.
+  */
+  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 list and create temp table
   */
-  const_fields= 0;
   always_null= 0;
   for (uint i= 0; i < arg_count_field; i++)
   {
@@ -2986,33 +2971,29 @@
       DBUG_RETURN(1);
     if (item->const_item())
     {
-      const_fields++;
       (void) item->val_int();
       if (item->null_value)
+      {
 	always_null= 1;
+        break;
+      }
     }
   }
   if (always_null)
     DBUG_RETURN(0);
 
   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);
-  }
+  /*
+    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 for tmp table columns.
+  */
+  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);
-  if (table)
-  {
-    /*
-      We come here when we are getting the result from a temporary table,
-      not the original tables used in the query
-    */
-    free_tmp_table(thd, table);
-    tmp_table_param->cleanup();
-  }
+  DBUG_ASSERT(table == 0);
   /*
     We have to create a temporary table to get descriptions of fields 
     (types, sizes and so on).
@@ -3031,18 +3012,17 @@
   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)
@@ -3061,20 +3041,9 @@
     */
     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);
 }
 
@@ -3083,10 +3052,10 @@
 
 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 +3065,17 @@
   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;
 }
 

--- 1.79/sql/item_sum.h	2005-03-15 03:46:13 +03:00
+++ 1.80/sql/item_sum.h	2005-03-17 03:02:56 +03:00
@@ -834,15 +834,26 @@
 
 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;
+  String result;
+  String *separator;
+  TREE tree_base;
+  TREE *tree;
+  TABLE *table;
+  ORDER **order;
+  TABLE_LIST *tables_list;
+  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);
@@ -854,30 +865,12 @@
   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;
-  TREE *tree;
-  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 count_cut_values;
-  /*
-    Following is 0 normal object and pointer to original one for copy 
-    (to correctly free resources)
-  */
-  Item_func_group_concat *original;
-  
+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 @@
   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);

--- 1.33/mysql-test/r/func_gconcat.result	2005-01-15 18:38:37 +03:00
+++ 1.34/mysql-test/r/func_gconcat.result	2005-03-17 03:02:56 +03:00
@@ -166,7 +166,7 @@
 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));