Bug #44399 : crash with statement using TEXT columns, aggregates, GROUP BY, and

HAVING
            
When calculating GROUP BY the server caches some expressions. It does
that by allocating a string slot (Item_copy_string) and assigning the 
value of the expression to it. This effectively means that the result
type of the expression can be changed from whatever it was to a string.
As this substitution takes place after the compile-time result type 
calculation for IN but before the run-time type calculations, 
it causes the type calculations in the IN function done at run time 
to get unexpected results different from what was prepared at compile time.
                  
In the CASE ... WHEN ... THEN ... statement there was a similar problem
and it was solved by artificially adding a STRING argument to the set of 
types of the IN/CASE arguments at compile time, so if any of the 
arguments of the CASE function changes its type to a string it will 
still be covered by the information prepared at compile time.

mysql-test/include/mix1.inc:
  Bug #44399: extended the test to cover the different types
mysql-test/r/func_in.result:
  Bug #44399: test case
mysql-test/r/innodb_mysql.result:
  Bug #44399: extended the test to cover the different types
mysql-test/t/func_in.test:
  Bug #44399: test case
sql/item.cc:
  Bug #44399: Implement typed caching for GROUP BY
sql/item.h:
  Bug #44399: Implement typed caching for GROUP BY
sql/item_cmpfunc.cc:
  Bug #44399: remove the special case
sql/sql_select.cc:
  Bug #44399: Implement typed caching for GROUP BY

mysql-test/include/mix1.inc

@@ -1544,4 +1544,31 @@ SELECT 1 FROM (SELECT COUNT(DISTINCT c1)
 
 DROP TABLE t1;
 
+eval
+CREATE TABLE t1 (c1 REAL, c2 REAL, c3 REAL, KEY (c3), KEY (c2, c3))
+  ENGINE=$engine_type;
+INSERT INTO t1 VALUES (1,1,1), (1,1,1), (1,1,2), (1,1,1), (1,1,2);
+
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+                 FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+EXPLAIN 
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+                 FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+
+DROP TABLE t1;
+
+eval
+CREATE TABLE t1 (c1 DECIMAL(12,2), c2 DECIMAL(12,2), c3 DECIMAL(12,2), 
+  KEY (c3), KEY (c2, c3))
+  ENGINE=$engine_type;
+INSERT INTO t1 VALUES (1,1,1), (1,1,1), (1,1,2), (1,1,1), (1,1,2);
+
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+                 FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+EXPLAIN 
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+                 FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+
+DROP TABLE t1;
+
 --echo End of 5.1 tests

mysql-test/r/func_in.result

@@ -587,4 +587,25 @@ SELECT CASE c1 WHEN c1 + 1 THEN 1 END, ABS(AVG(c0)) FROM t1;
 CASE c1 WHEN c1 + 1 THEN 1 END	ABS(AVG(c0))
 NULL	1.0000
 DROP TABLE t1;
+CREATE TABLE t1(a TEXT, b INT, c INT UNSIGNED, d DECIMAL(12,2), e REAL);
+INSERT INTO t1 VALUES('iynfj', 1, 1, 1, 1);
+INSERT INTO t1 VALUES('innfj', 2, 2, 2, 2);
+SELECT SUM( DISTINCT a ) FROM t1 GROUP BY a HAVING a IN ( AVG( 1 ), 1 + a);
+SUM( DISTINCT a )
+SELECT SUM( DISTINCT b ) FROM t1 GROUP BY b HAVING b IN ( AVG( 1 ), 1 + b);
+SUM( DISTINCT b )
+1
+SELECT SUM( DISTINCT c ) FROM t1 GROUP BY c HAVING c IN ( AVG( 1 ), 1 + c);
+SUM( DISTINCT c )
+1
+SELECT SUM( DISTINCT d ) FROM t1 GROUP BY d HAVING d IN ( AVG( 1 ), 1 + d);
+SUM( DISTINCT d )
+1.00
+SELECT SUM( DISTINCT e ) FROM t1 GROUP BY e HAVING e IN ( AVG( 1 ), 1 + e);
+SUM( DISTINCT e )
+1
+SELECT SUM( DISTINCT e ) FROM t1 GROUP BY b,c,d HAVING (b,c,d) IN 
+((AVG( 1 ), 1 + c, 1 + d), (AVG( 1 ), 2 + c, 2 + d));
+SUM( DISTINCT e )
+DROP TABLE t1;
 End of 5.1 tests

mysql-test/r/innodb_mysql.result

@@ -1718,6 +1718,35 @@ id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
 1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
 2	DERIVED	t1	index	c3,c2	c2	10	NULL	5	
 DROP TABLE t1;
+CREATE TABLE t1 (c1 REAL, c2 REAL, c3 REAL, KEY (c3), KEY (c2, c3))
+ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1,1,1), (1,1,1), (1,1,2), (1,1,1), (1,1,2);
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+1
+1
+EXPLAIN 
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
+2	DERIVED	t1	index	c3,c2	c2	18	NULL	5	
+DROP TABLE t1;
+CREATE TABLE t1 (c1 DECIMAL(12,2), c2 DECIMAL(12,2), c3 DECIMAL(12,2), 
+KEY (c3), KEY (c2, c3))
+ENGINE=InnoDB;
+INSERT INTO t1 VALUES (1,1,1), (1,1,1), (1,1,2), (1,1,1), (1,1,2);
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+1
+1
+EXPLAIN 
+SELECT 1 FROM (SELECT COUNT(DISTINCT c1) 
+FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
+2	DERIVED	t1	index	c3,c2	c2	14	NULL	5	
+DROP TABLE t1;
 End of 5.1 tests
 drop table if exists t1, t2, t3;
 create table t1(a int);

mysql-test/t/func_in.test

@@ -439,4 +439,21 @@ SELECT CASE c1 WHEN c1 + 1 THEN 1 END, ABS(AVG(c0)) FROM t1;
 
 DROP TABLE t1;
 
+#
+# Bug #44399: crash with statement using TEXT columns, aggregates, GROUP BY, 
+# and HAVING
+#
+
+CREATE TABLE t1(a TEXT, b INT, c INT UNSIGNED, d DECIMAL(12,2), e REAL);
+INSERT INTO t1 VALUES('iynfj', 1, 1, 1, 1);
+INSERT INTO t1 VALUES('innfj', 2, 2, 2, 2);
+SELECT SUM( DISTINCT a ) FROM t1 GROUP BY a HAVING a IN ( AVG( 1 ), 1 + a);
+SELECT SUM( DISTINCT b ) FROM t1 GROUP BY b HAVING b IN ( AVG( 1 ), 1 + b);
+SELECT SUM( DISTINCT c ) FROM t1 GROUP BY c HAVING c IN ( AVG( 1 ), 1 + c);
+SELECT SUM( DISTINCT d ) FROM t1 GROUP BY d HAVING d IN ( AVG( 1 ), 1 + d);
+SELECT SUM( DISTINCT e ) FROM t1 GROUP BY e HAVING e IN ( AVG( 1 ), 1 + e);
+SELECT SUM( DISTINCT e ) FROM t1 GROUP BY b,c,d HAVING (b,c,d) IN 
+  ((AVG( 1 ), 1 + c, 1 + d), (AVG( 1 ), 2 + c, 2 + d));
+DROP TABLE t1;
+
 --echo End of 5.1 tests

sql/item.cc

@@ -3268,10 +3268,58 @@ Item_param::set_param_type_and_swap_value(Item_param *src)
   str_value_ptr.swap(src->str_value_ptr);
 }
 
+/****************************************************************************
+  Item_copy
+****************************************************************************/
+Item_copy *Item_copy::create (Item *item)
+{
+  switch (item->result_type())
+  {
+    case STRING_RESULT:
+      return new Item_copy_string (item);
+    case REAL_RESULT: 
+      return new Item_copy_float (item);
+    case INT_RESULT:
+      return item->unsigned_flag ? 
+        new Item_copy_uint (item) : new Item_copy_int (item);
+    case DECIMAL_RESULT:
+      return new Item_copy_decimal (item);
+
+    case ROW_RESULT:
+      DBUG_ASSERT (0);
+  }
+  /* should not happen */
+  return NULL;
+}
+
 /****************************************************************************
   Item_copy_string
 ****************************************************************************/
 
+double Item_copy_string::val_real()
+{
+  int err_not_used;
+  char *end_not_used;
+  return (null_value ? 0.0 :
+          my_strntod(str_value.charset(), (char*) str_value.ptr(),
+                     str_value.length(), &end_not_used, &err_not_used));
+}
+
+longlong Item_copy_string::val_int()
+{
+  int err;
+  return null_value ? LL(0) : my_strntoll(str_value.charset(),str_value.ptr(),
+                                          str_value.length(),10, (char**) 0,
+                                          &err); 
+}
+
+
+int Item_copy_string::save_in_field(Field *field, bool no_conversions)
+{
+  return save_str_value_in_field(field, &str_value);
+}
+
+
 void Item_copy_string::copy()
 {
   String *res=item->val_str(&str_value);
@@ -3294,12 +3342,163 @@ my_decimal *Item_copy_string::val_decimal(my_decimal *decimal_value)
 {
   // Item_copy_string is used without fix_fields call
   if (null_value)
-    return 0;
+    return (my_decimal *) 0;
   string2my_decimal(E_DEC_FATAL_ERROR, &str_value, decimal_value);
   return (decimal_value);
 }
 
 
+/****************************************************************************
+  Item_copy_int
+****************************************************************************/
+
+void Item_copy_int::copy()
+{
+  cached_value= item->val_int();
+  null_value=item->null_value;
+}
+
+static int save_int_value_in_field (Field *field, longlong nr, 
+                                    bool null_value, bool unsigned_flag);
+
+int Item_copy_int::save_in_field(Field *field, bool no_conversions)
+{
+  return save_int_value_in_field(field, cached_value, 
+                                 null_value, unsigned_flag);
+}
+
+
+String *Item_copy_int::val_str(String *str)
+{
+  if (null_value)
+    return (String *) 0;
+
+  str->set(cached_value, &my_charset_bin);
+  return str;
+}
+
+
+my_decimal *Item_copy_int::val_decimal(my_decimal *decimal_value)
+{
+  if (null_value)
+    return (my_decimal *) 0;
+
+  int2my_decimal(E_DEC_FATAL_ERROR, cached_value, unsigned_flag, decimal_value);
+  return decimal_value;
+}
+
+
+/****************************************************************************
+  Item_copy_uint
+****************************************************************************/
+
+String *Item_copy_uint::val_str(String *str)
+{
+  if (null_value)
+    return (String *) 0;
+
+  str->set((ulonglong) cached_value, &my_charset_bin);
+  return str;
+}
+
+
+/****************************************************************************
+  Item_copy_float
+****************************************************************************/
+
+String *Item_copy_float::val_str(String *str)
+{
+  if (null_value)
+    return (String *) 0;
+  else
+  {
+    double nr= val_real();
+    str->set_real(nr,decimals, &my_charset_bin);
+    return str;
+  }
+}
+
+
+my_decimal *Item_copy_float::val_decimal(my_decimal *decimal_value)
+{
+  if (null_value)
+    return (my_decimal *) 0;
+  else
+  {
+    double nr= val_real();
+    double2my_decimal(E_DEC_FATAL_ERROR, nr, decimal_value);
+    return decimal_value;
+  }
+}
+
+
+int Item_copy_float::save_in_field(Field *field, bool no_conversions)
+{
+  if (null_value)
+    return set_field_to_null(field);
+  field->set_notnull();
+  return field->store(cached_value);
+}
+
+
+/****************************************************************************
+  Item_copy_decimal
+****************************************************************************/
+
+int Item_copy_decimal::save_in_field(Field *field, bool no_conversions)
+{
+  if (null_value)
+    return set_field_to_null(field);
+  field->set_notnull();
+  return field->store_decimal(&cached_value);
+}
+
+
+String *Item_copy_decimal::val_str(String *result)
+{
+  if (null_value)
+    return (String *) 0;
+  result->set_charset(&my_charset_bin);
+  my_decimal2string(E_DEC_FATAL_ERROR, &cached_value, 0, 0, 0, result);
+  return result;
+}
+
+
+double Item_copy_decimal::val_real()
+{
+  if (null_value)
+    return 0.0;
+  else
+  {
+    double result;
+    my_decimal2double(E_DEC_FATAL_ERROR, &cached_value, &result);
+    return result;
+  }
+}
+
+
+longlong Item_copy_decimal::val_int()
+{
+  if (null_value)
+    return LL(0);
+  else
+  {
+    longlong result;
+    my_decimal2int(E_DEC_FATAL_ERROR, &cached_value, unsigned_flag, &result);
+    return result;
+  }
+}
+
+
+void Item_copy_decimal::copy()
+{
+  my_decimal *nr= item->val_decimal(&cached_value);
+  if (nr && nr != &cached_value)
+    memcpy (&cached_value, nr, sizeof (my_decimal)); 
+  null_value= item->null_value;
+}
+
+
 /*
   Functions to convert item to field (for send_fields)
 */
@@ -4947,10 +5146,9 @@ int Item_uint::save_in_field(Field *field, bool no_conversions)
   return Item_int::save_in_field(field, no_conversions);
 }
 
-
-int Item_int::save_in_field(Field *field, bool no_conversions)
+static int save_int_value_in_field (Field *field, longlong nr, 
+                                    bool null_value, bool unsigned_flag)
 {
-  longlong nr=val_int();
   if (null_value)
     return set_field_to_null(field);
   field->set_notnull();
@@ -4958,6 +5156,12 @@ int Item_int::save_in_field(Field *field, bool no_conversions)
 }
 
 
+int Item_int::save_in_field(Field *field, bool no_conversions)
+{
+  return save_int_value_in_field (field, val_int(), null_value, unsigned_flag);
+}
+
+
 int Item_decimal::save_in_field(Field *field, bool no_conversions)
 {
   field->set_notnull();

sql/item.h

@@ -2443,48 +2443,203 @@ public:
 #include "item_xmlfunc.h"
 #endif
 
-class Item_copy_string :public Item
+/**
+  Base class to implement typed value caching Item classes
+
+  Item_copy_ classes are very similar to the corresponding Item_
+  classes (e.g. Item_copy_int is similar to Item_int) but they add
+  the following additional functionality to Item_ :
+    1. Nullability
+    2. Possibility to store the value not only on instantiation time,
+       but also later.
+  Item_copy_ classes are a functionality subset of Item_cache_ 
+  classes, as e.g. they don't support comparisons with the original Item
+  as Item_cache_ classes do.
+  Item_copy_ classes are used in GROUP BY calculation.
+  TODO: Item_copy should be made an abstract interface and Item_copy_
+  classes should inherit both the respective Item_ class and the interface.
+  Ideally we should drop Item_copy_ classes altogether and merge 
+  their functionality to Item_cache_ (and these should be made to inherit
+  from Item_).
+*/
+
+class Item_copy :public Item
 {
+protected:  
+
+  /**
+    Stores the type of the resulting field that would be used to store the data
+    in the cache. This is to avoid calls to the original item.
+  */
   enum enum_field_types cached_field_type;
-public:
+
+  /** The original item that is copied */
   Item *item;
-  Item_copy_string(Item *i) :item(i)
+
+  /**
+    Stores the result type of the original item, so it can be returned
+    without calling the original item's method
+  */
+  Item_result cached_result_type;
+
+  /**
+    Constructor of the Item_copy class
+
+    stores metadata information about the original class as well as a 
+    pointer to it.
+  */
+  Item_copy(Item *i)
   {
+    item= i;
     null_value=maybe_null=item->maybe_null;
     decimals=item->decimals;
     max_length=item->max_length;
     name=item->name;
     cached_field_type= item->field_type();
+    cached_result_type= item->result_type();
+    unsigned_flag= item->unsigned_flag;
   }
+
+public:
+  /** 
+    Factory method to create the appropriate subclass dependent on the type of 
+    the original item.
+
+    @param item      the original item.
+  */  
+  static Item_copy *create (Item *item);
+
+  /** 
+    Update the cache with the value of the original item
+   
+    This is the method that updates the cached value.
+    It must be explicitly called by the user of this class to store the value 
+    of the orginal item in the cache.
+  */  
+  virtual void copy() = 0;
+
+  Item *get_item() { return item; }
+  /** All of the subclasses should have the same type tag */
   enum Type type() const { return COPY_STR_ITEM; }
-  enum Item_result result_type () const { return STRING_RESULT; }
   enum_field_types field_type() const { return cached_field_type; }
-  double val_real()
+  enum Item_result result_type () const { return cached_result_type; }
+
+  void make_field(Send_field *field) { item->make_field(field); }
+  table_map used_tables() const { return (table_map) 1L; }
+  bool const_item() const { return 0; }
+  bool is_null() { return null_value; }
+
+  /*  
+    Override the methods below as pure virtual to make sure all the 
+    sub-classes implement them.
+  */  
+
+  virtual String *val_str(String*) = 0;
+  virtual my_decimal *val_decimal(my_decimal *) = 0;
+  virtual double val_real() = 0;
+  virtual longlong val_int() = 0;
+  virtual int save_in_field(Field *field, bool no_conversions) = 0;
+};
+
+/**
+ Implementation of a string cache.
+ 
+ Uses Item::str_value for storage
+*/ 
+class Item_copy_string : public Item_copy
+{
+public:
+  Item_copy_string (Item *item) : Item_copy(item) {}
+
+  String *val_str(String*);
+  my_decimal *val_decimal(my_decimal *);
+  double val_real();
+  longlong val_int();
+  void copy();
+  int save_in_field(Field *field, bool no_conversions);
+};
+
+
+class Item_copy_int : public Item_copy
+{
+protected:  
+  longlong cached_value; 
+public:
+  Item_copy_int (Item *i) : Item_copy(i) {}
+  int save_in_field(Field *field, bool no_conversions);
+
+  virtual String *val_str(String*);
+  virtual my_decimal *val_decimal(my_decimal *);
+  virtual double val_real()
   {
-    int err_not_used;
-    char *end_not_used;
-    return (null_value ? 0.0 :
-	    my_strntod(str_value.charset(), (char*) str_value.ptr(),
-		       str_value.length(), &end_not_used, &err_not_used));
+    return null_value ? 0.0 : (double) cached_value;
   }
-  longlong val_int()
+  virtual longlong val_int()
+  {
+    return null_value ? LL(0) : cached_value;
+  }
+  virtual void copy();
+};
+
+
+class Item_copy_uint : public Item_copy_int
+{
+public:
+  Item_copy_uint (Item *item) : Item_copy_int(item) 
+  {
+    unsigned_flag= 1;
+  }
+
+  String *val_str(String*);
+  double val_real()
   {
-    int err;
-    return null_value ? LL(0) : my_strntoll(str_value.charset(),str_value.ptr(),
-                                            str_value.length(),10, (char**) 0,
-                                            &err); 
+    return null_value ? 0.0 : (double) (ulonglong) cached_value;
   }
+};
+
+
+class Item_copy_float : public Item_copy
+{
+protected:  
+  double cached_value; 
+public:
+  Item_copy_float (Item *i) : Item_copy(i) {}
+  int save_in_field(Field *field, bool no_conversions);
+
   String *val_str(String*);
   my_decimal *val_decimal(my_decimal *);
-  void make_field(Send_field *field) { item->make_field(field); }
-  void copy();
-  int save_in_field(Field *field, bool no_conversions)
+  double val_real()
   {
-    return save_str_value_in_field(field, &str_value);
+    return null_value ? 0.0 : cached_value;
   }
-  table_map used_tables() const { return (table_map) 1L; }
-  bool const_item() const { return 0; }
-  bool is_null() { return null_value; }
+  longlong val_int()
+  {
+    return (longlong) rint(val_real());
+  }
+  void copy()
+  {
+    cached_value= item->val_real();
+    null_value= item->null_value;
+  }
+};
+
+
+class Item_copy_decimal : public Item_copy
+{
+protected:  
+  my_decimal cached_value;
+public:
+  Item_copy_decimal (Item *i) : Item_copy(i) {}
+  int save_in_field(Field *field, bool no_conversions);
+
+  String *val_str(String*);
+  my_decimal *val_decimal(my_decimal *) 
+  { 
+    return null_value ? NULL: &cached_value; 
+  }
+  double val_real();
+  longlong val_int();
+  void copy();
 };
 
 

sql/item_cmpfunc.cc

@@ -2724,16 +2724,6 @@ void Item_func_case::fix_length_and_dec()
     nagg++;
     if (!(found_types= collect_cmp_types(agg, nagg)))
       return;
-    if (with_sum_func || current_thd->lex->current_select->group_list.elements)
-    {
-      /*
-        See TODO commentary in the setup_copy_fields function:
-        item in a group may be wrapped with an Item_copy_string item.
-        That item has a STRING_RESULT result type, so we need
-        to take this type into account.
-      */
-      found_types |= (1 << item_cmp_type(left_result_type, STRING_RESULT));
-    }
 
     for (i= 0; i <= (uint)DECIMAL_RESULT; i++)
     {

sql/sql_select.cc

@@ -13855,7 +13855,7 @@ SORT_FIELD *make_unireg_sortorder(ORDER *order, uint *length,
       pos->field= ((Item_sum*) item)->get_tmp_table_field();
     else if (item->type() == Item::COPY_STR_ITEM)
     {						// Blob patch
-      pos->item= ((Item_copy_string*) item)->item;
+      pos->item= ((Item_copy*) item)->get_item();
     }
     else
       pos->item= *order->item;
@@ -14926,7 +14926,7 @@ setup_copy_fields(THD *thd, TMP_TABLE_PARAM *param,
       pos= item;
       if (item->field->flags & BLOB_FLAG)
       {
-	if (!(pos= new Item_copy_string(pos)))
+	if (!(pos= Item_copy::create(pos)))
 	  goto err;
        /*
          Item_copy_string::copy for function can call 
@@ -14980,7 +14980,7 @@ setup_copy_fields(THD *thd, TMP_TABLE_PARAM *param,
 	 on how the value is to be used: In some cases this may be an
 	 argument in a group function, like: IF(ISNULL(col),0,COUNT(*))
       */
-      if (!(pos=new Item_copy_string(pos)))
+      if (!(pos= Item_copy::create(pos)))
 	goto err;
       if (i < border)                           // HAVING, ORDER and GROUP BY
       {
@@ -15033,8 +15033,8 @@ copy_fields(TMP_TABLE_PARAM *param)
     (*ptr->do_copy)(ptr);
 
   List_iterator_fast<Item> it(param->copy_funcs);
-  Item_copy_string *item;
-  while ((item = (Item_copy_string*) it++))
+  Item_copy *item;
+  while ((item = (Item_copy*) it++))
     item->copy();
 }