Better bugfix for "HAVING when refering to RAND()" (Bug #8216)

Ensure that references in HAVING, ORDER BY or GROUP BY are calculated after fields in SELECT.
This will ensure that any reference to these has a valid value.
Generalized the code for split_sum_func()


BitKeeper/etc/ignore:
  added support-files/ndb-config-2-node.ini
mysql-test/r/group_by.result:
  More complicated test to assure that rand() is only calulated once
mysql-test/r/user_var.result:
  Back to old results :(  (ok but not perfect)
mysql-test/t/group_by.test:
  More complicated test to assure that rand() is only calulated once
sql/item.cc:
  Better bugfix for "HAVING when refering to RAND()"
  This will ensure that when refering to things like RAND() in HAVING through an alias we will not recalculate that rand() value in the HAVING part but use the value in the row
  Generalize split_sum_func()
sql/item.h:
  Better bugfix for "HAVING when refering to RAND()"
  T
sql/item_cmpfunc.cc:
  Better bugfix for "HAVING when refering to RAND()"
  Use generalized split_sum_func2() function
sql/item_func.cc:
  Better bugfix for "HAVING when refering to RAND()"
  Use generalized split_sum_func2() function
sql/item_row.cc:
  Better bugfix for "HAVING when refering to RAND()"
  Use generalized split_sum_func2() function
sql/item_strfunc.cc:
  Better bugfix for "HAVING when refering to RAND()"
  Use generalized split_sum_func2() function
sql/sql_list.h:
  Add functions to concatenate lists
sql/sql_select.cc:
  Better bugfix for "HAVING when refering to RAND()"
  Ensure that references in HAVING, ORDER BY or GROUP BY are calculated after fields in SELECT.
  This will ensure that any reference to these has a valid value.

.bzrignore

@@ -1007,3 +1007,4 @@ tests/mysql_client_test
 libmysqld/examples/mysql_client_test.c
 libmysqld/examples/mysql_client_test_embedded
 libmysqld/examples/mysqltest_embedded
+support-files/ndb-config-2-node.ini

mysql-test/r/group_by.result

@@ -655,9 +655,9 @@ insert into t1 (a,b) values (1,2),(1,3),(2,5);
 select a, 0.1*0+1 r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2=1;
 a	r2	r1
 1	1.0	2
-select a, rand()*0+1 r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2=1;
+select a, round(rand(100)*10) r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2<=2;
 a	r2	r1
-1	1	2
+1	2	2
 select a,sum(b) from t1 where a=1 group by c;
 a	sum(b)
 1	5

mysql-test/r/user_var.result

@@ -109,8 +109,8 @@ select @a:=0;
 select @a, @a:=@a+count(*), count(*), @a from t1 group by i;
 @a	@a:=@a+count(*)	count(*)	@a
 0	1	1	0
-0	3	2	0
-0	6	3	0
+0	2	2	0
+0	3	3	0
 select @a:=0;
 @a:=0
 0

mysql-test/t/group_by.test

@@ -480,7 +480,8 @@ drop table t1;
 create table t1 (a integer, b integer, c integer);
 insert into t1 (a,b) values (1,2),(1,3),(2,5);
 select a, 0.1*0+1 r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2=1;
-select a, rand()*0+1 r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2=1;
+# rand(100)*10 will be < 2 only for the first row (of 6)
+select a, round(rand(100)*10) r2, sum(1) r1 from t1 where a = 1 group  by a having r1>1 and r2<=2;
 select a,sum(b) from t1 where a=1 group by c;
 select a*sum(b) from t1 where a=1 group by c;
 select sum(a)*sum(b) from t1 where a=1 group by c;

sql/item.cc

@@ -295,6 +295,58 @@ CHARSET_INFO *Item::default_charset()
 }
 
 
+/*
+  Move SUM items out from item tree and replace with reference
+
+  SYNOPSIS
+    split_sum_func2()
+    thd			Thread handler
+    ref_pointer_array	Pointer to array of reference fields
+    fields		All fields in select
+    ref			Pointer to item
+
+  NOTES
+   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.
+
+   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)
+{
+  if (type() != SUM_FUNC_ITEM && with_sum_func)
+  {
+    /* Will split complicated items and ignore simple ones */
+    split_sum_func(thd, ref_pointer_array, fields);
+  }
+  else if ((type() == SUM_FUNC_ITEM ||
+            (used_tables() & ~PARAM_TABLE_BIT)) &&
+           type() != REF_ITEM)
+  {
+    /*
+      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)
+    */
+    uint el= fields.elements;
+    Item *new_item;    
+    ref_pointer_array[el]= this;
+    if (!(new_item= new Item_ref(ref_pointer_array + el, 0, name)))
+      return;                                   // fatal_error is set
+    fields.push_front(this);
+    ref_pointer_array[el]= this;
+    thd->change_item_tree(ref, new_item);
+  }
+}
+
+
 /*
    Aggregate two collations together taking
    into account their coercibility (aka derivation):

sql/item.h

@@ -262,6 +262,9 @@ class Item {
   virtual void update_used_tables() {}
   virtual void split_sum_func(THD *thd, Item **ref_pointer_array,
                               List<Item> &fields) {}
+  /* Called for items that really have to be split */
+  void split_sum_func2(THD *thd, Item **ref_pointer_array, List<Item> &fields,
+                       Item **ref);
   virtual bool get_date(TIME *ltime,uint fuzzydate);
   virtual bool get_time(TIME *ltime);
   virtual bool get_date_result(TIME *ltime,uint fuzzydate)

sql/item_cmpfunc.cc

@@ -1969,10 +1969,10 @@ bool Item_cond::walk(Item_processor processor, byte *arg)
   Move SUM items out from item tree and replace with reference
 
   SYNOPSIS
-  split_sum_func()
-  thd			Thread handler
-  ref_pointer_array	Pointer to array of reference fields
-  fields		All fields in select
+    split_sum_func()
+    thd			Thread handler
+    ref_pointer_array	Pointer to array of reference fields
+    fields		All fields in select
 
   NOTES
    This function is run on all expression (SELECT list, WHERE, HAVING etc)
@@ -1982,16 +1982,6 @@ bool Item_cond::walk(Item_processor processor, byte *arg)
    so that we can easily find and calculate them.
    (Calculation done by update_sum_func() and copy_sum_funcs() in
    sql_select.cc)
-
-   All found SUM items are added FIRST in the fields list and
-   we replace the item with a reference.
-
-   We also replace all functions without side effects (like RAND() or UDF's)
-   that uses columns as arguments.
-   For functions with side effects, we just remember any fields referred
-   by the function to ensure that we get a copy of the field value for the
-   first accepted row. This ensures that we can do things like
-   SELECT a*SUM(b) FROM t1 WHERE a=1
 */
 
 void Item_cond::split_sum_func(THD *thd, Item **ref_pointer_array,
@@ -1999,38 +1989,8 @@ void Item_cond::split_sum_func(THD *thd, Item **ref_pointer_array,
 {
   List_iterator<Item> li(list);
   Item *item;
-  used_tables_cache=0;
-  const_item_cache=0;
-  while ((item=li++))
-  {
-    /* with_sum_func is set for items that contains a SUM expression */
-    if (item->type() != SUM_FUNC_ITEM &&
-        (item->with_sum_func ||
-         (item->used_tables() & PSEUDO_TABLE_BITS)))
-      item->split_sum_func(thd, ref_pointer_array, fields);
-    else if (item->type() == SUM_FUNC_ITEM ||
-             (item->used_tables() && item->type() != REF_ITEM))
-    {
-      /*
-        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 or are not yet calculated as in:
-        SELECT RAND() as r1, SUM(a) as r2 FROM t1 HAVING r1 > 1 AND r2 > 0
-      */
-      Item **ref= li.ref();
-      uint el= fields.elements;
-      ref_pointer_array[el]= item;
-      Item *new_item= new Item_ref(ref_pointer_array + el, 0, item->name);
-      fields.push_front(item);
-      ref_pointer_array[el]= item;
-      thd->change_item_tree(ref, new_item);
-    }
-    item->update_used_tables();
-    used_tables_cache|=item->used_tables();
-    const_item_cache&=item->const_item();
-  }
+  while ((item= li++))
+    item->split_sum_func2(thd, ref_pointer_array, fields, li.ref());
 }
 
 

sql/item_func.cc

@@ -352,28 +352,14 @@ bool Item_func::walk (Item_processor processor, byte *argument)
 }
 
 
+/* See comments in Item_cmp_func::split_sum_func() */
+
 void Item_func::split_sum_func(THD *thd, Item **ref_pointer_array,
                                List<Item> &fields)
 {
   Item **arg, **arg_end;
   for (arg= args, arg_end= args+arg_count; arg != arg_end ; arg++)
-  {
-    Item *item=* arg;
-    if (item->type() != SUM_FUNC_ITEM &&
-        (item->with_sum_func ||
-         (item->used_tables() & PSEUDO_TABLE_BITS)))
-      item->split_sum_func(thd, ref_pointer_array, fields);
-    else if (item->type() == SUM_FUNC_ITEM ||
-             (item->used_tables() && item->type() != REF_ITEM))
-    {
-      uint el= fields.elements;
-      ref_pointer_array[el]= item;
-      Item *new_item= new Item_ref(ref_pointer_array + el, 0, item->name);
-      fields.push_front(item);
-      ref_pointer_array[el]= item;
-      thd->change_item_tree(arg, new_item);
-    }
-  }
+    (*arg)->split_sum_func2(thd, ref_pointer_array, fields, arg);
 }
 
 

sql/item_row.cc

@@ -90,25 +90,10 @@ void Item_row::split_sum_func(THD *thd, Item **ref_pointer_array,
 {
   Item **arg, **arg_end;
   for (arg= items, arg_end= items+arg_count; arg != arg_end ; arg++)
-  {
-    Item *item= *arg;
-    if (item->type() != SUM_FUNC_ITEM &&
-        (item->with_sum_func ||
-         (item->used_tables() & PSEUDO_TABLE_BITS)))
-      item->split_sum_func(thd, ref_pointer_array, fields);
-    else if (item->type() == SUM_FUNC_ITEM ||
-             (item->used_tables() && item->type() != REF_ITEM))
-    {
-      uint el= fields.elements;
-      ref_pointer_array[el]=*arg;
-      Item *new_item= new Item_ref(ref_pointer_array + el, 0, (*arg)->name);
-      fields.push_front(*arg);
-      ref_pointer_array[el]= *arg;
-      thd->change_item_tree(arg, new_item);
-    }
-  }
+    (*arg)->split_sum_func2(thd, ref_pointer_array, fields, arg);
 }
 
+
 void Item_row::update_used_tables()
 {
   used_tables_cache= 0;

sql/item_strfunc.cc

@@ -1758,20 +1758,7 @@ String *Item_func_elt::val_str(String *str)
 void Item_func_make_set::split_sum_func(THD *thd, Item **ref_pointer_array,
 					List<Item> &fields)
 {
-  if (item->type() != SUM_FUNC_ITEM &&
-      (item->with_sum_func ||
-       (item->used_tables() & PSEUDO_TABLE_BITS)))
-    item->split_sum_func(thd, ref_pointer_array, fields);
-  else if (item->type() == SUM_FUNC_ITEM ||
-           (item->used_tables() && item->type() != REF_ITEM))
-  {
-    uint el= fields.elements;
-    ref_pointer_array[el]=item;
-    Item *new_item= new Item_ref(ref_pointer_array + el, 0, item->name);
-    fields.push_front(item);
-    ref_pointer_array[el]= item;
-    thd->change_item_tree(&item, new_item);
-  }
+  item->split_sum_func2(thd, ref_pointer_array, fields, &item);
   Item_str_func::split_sum_func(thd, ref_pointer_array, fields);
 }
 

sql/sql_list.h

@@ -134,6 +134,15 @@ class base_list :public Sql_alloc
     if (!--elements)
       last= &first;
   }
+  inline void concat(base_list *list)
+  {
+    if (!list->is_empty())
+    {
+      *last= list->first;
+      last= list->last;
+      elements+= list->elements;
+    }
+  }
   inline void *pop(void)
   {
     if (first == &end_of_list) return 0;

sql/sql_select.cc

@@ -8502,6 +8502,7 @@ setup_copy_fields(THD *thd, TMP_TABLE_PARAM *param,
   res_selected_fields.empty();
   res_all_fields.empty();
   List_iterator_fast<Item> itr(res_all_fields);
+  List<Item> extra_funcs;
   uint i, border= all_fields.elements - elements;
   DBUG_ENTER("setup_copy_fields");
 
@@ -8563,7 +8564,12 @@ setup_copy_fields(THD *thd, TMP_TABLE_PARAM *param,
       */
       if (!(pos=new Item_copy_string(pos)))
 	goto err;
-      if (param->copy_funcs.push_back(pos))
+      if (i < border)                           // HAVING, ORDER and GROUP BY
+      {
+        if (extra_funcs.push_back(pos))
+          goto err;
+      }
+      else if (param->copy_funcs.push_back(pos))
 	goto err;
     }
     res_all_fields.push_back(pos);
@@ -8575,6 +8581,12 @@ setup_copy_fields(THD *thd, TMP_TABLE_PARAM *param,
   for (i= 0; i < border; i++)
     itr++;
   itr.sublist(res_selected_fields, elements);
+  /*
+    Put elements from HAVING, ORDER BY and GROUP BY last to ensure that any
+    reference used in these will resolve to a item that is already calculated
+  */
+  param->copy_funcs.concat(&extra_funcs);
+
   DBUG_RETURN(0);
 
  err: