maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,

It appeared to be very easy to fix all hybrid functions to use the same
attribute aggregation code, and therefore get rid of duplicate
implementations for Item_func_case_abbreviation2 and
Item_func_case.



This patch fixes:
- The original problem reported in MDEV-9653

- An additional problem, see new comments about COALESCE and IF
  returning 1.1000000000000000000000000000000 instead of just 1.1

- A new problem that I found:
  MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements


Note, the patch does not cover LEAST/GREATEST. They have another
version of their own aggregation code. I don't want to touch them
in this fix. Let's do them separately.


Note, I also removed Item_func_case::agg_str_lengths().
It was a dead code.

Thanks.


On 03/17/2016 03:06 PM, Alexander Barkov wrote:
> Hi Sergei,
> 
> On 03/17/2016 01:14 PM, Sergei Golubchik wrote:
>> Hi, Alexander!
>>
>> On Mar 17, Alexander Barkov wrote:
>>>>
>>>> why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here.
>>>
>>> Perhaps my example in the comments made you think that
>>> it happens in a very special case with CAST AS UNSIGNED.
>>> It also happens in simpler cases:
>>>
>>> SELECT CASE WHEN TRUE
>>> THEN COALESCE(NULL)
>>> ELSE 40
>>> END;
>>>
>>> I guess I should fix the example to this ^^^.
>>
>> Yes, please.
>>
>> More questions:
>>
>> 1. Perhaps my_decimal_length_to_precision() then? It's used not only in
>>    Item_func_case::agg_num_lengths. Quick grepping finds more
>>    potentially dangerous places. For example
>>    Item_decimal_typecast::print.
> 
> I could not find places where my_decimal_length_to_precision() can be
> called with (0,NOT_FIXED_DEC).
> 
> Item_decimal_typecast::print() is safe.
> Item_decimal_typecast::max_length and Item_decimal_typecast::decimals
> are set to valid values in constructor. The latter cannot be NOT_FIXED_DEC.
> 
> 
>>
>> 2. Why my_decimal_length_to_precision is only used in Item_func_case?
>>    How do other functions aggregate types? Perhaps CASE needs
>>    to be changed to work like other similar functions do?
> 
> There is some code duplication in hybrid type functions:
> 
> - Item_func_case::agg_num_lengths()
> 
> - Item_func::count_decimal_length(), which is called from
>   Item_func_coalesce::fix_length_and_dec()
> 
> - Item_func_case_abbreviation2::fix_length_and_dec()
> 
> I earlier created a task for this for 10.2:
> https://jira.mariadb.org/browse/MDEV-9406
> 
> 
> I found new bugs when replying to this letter:
> 
> SELECT COALESCE(COALESCE(NULL), 1.1);
> SELECT IF(0, COALESCE(NULL), 1.1);
> 
> return 1.1000000000000000000000000000000, which is wrong.
> 
> I'll come up with a new patch soon.
> 
> Thanks.
> 
> 
>>
>> Regards,
>> Sergei
>> Chief Architect MariaDB
>> and security@xxxxxxxxxxx
>>

diff --git a/mysql-test/r/func_hybrid_type.result b/mysql-test/r/func_hybrid_type.result
index 95a8a82..eeaa79e 100644
--- a/mysql-test/r/func_hybrid_type.result
+++ b/mysql-test/r/func_hybrid_type.result
@@ -2175,10 +2175,10 @@ def					case_____a_b	12	19	19	Y	128	0	63
 def					case_____b_a	12	19	19	Y	128	0	63
 def					coalesce_a_b	12	19	19	Y	128	0	63
 def					coalesce_b_a	12	19	19	Y	128	0	63
-def					if_______a_b	12	10	19	Y	128	0	63
-def					if_______b_a	12	10	19	Y	128	0	63
-def					ifnull___a_b	12	10	19	Y	128	0	63
-def					ifnull___b_a	12	10	19	Y	128	0	63
+def					if_______a_b	12	19	19	Y	128	0	63
+def					if_______b_a	12	19	19	Y	128	0	63
+def					ifnull___a_b	12	19	19	Y	128	0	63
+def					ifnull___b_a	12	19	19	Y	128	0	63
 def					least____a_b	12	10	19	Y	128	0	63
 def					least____b_a	12	10	19	Y	128	0	63
 def					greatest_a_b	12	10	19	Y	128	0	63
@@ -3396,5 +3396,36 @@ c1
 DROP TABLE t2;
 DROP TABLE t1;
 #
+# MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
+#
+SELECT CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END;
+CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END
+NULL
+SELECT CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END;
+CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END
+NULL
+SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END;
+CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END
+NULL
+SELECT COALESCE(COALESCE(NULL), 1.1) AS c0, IF(0, COALESCE(NULL), 1.1) AS c1;
+Catalog	Database	Table	Table_alias	Column	Column_alias	Type	Length	Max length	Is_null	Flags	Decimals	Charsetnr
+def					c0	246	4	3	Y	32896	1	63
+def					c1	246	4	3	Y	32896	1	63
+c0	c1
+1.1	1.1
+#
+# MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements
+#
+PREPARE stmt FROM "CREATE TABLE t1 AS SELECT CONCAT(COALESCE(?,1)) AS a, CONCAT(CASE WHEN TRUE THEN ? ELSE 1 END) AS b";
+SET @a=1;
+EXECUTE stmt USING @a,@a;
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `a` varchar(21) DEFAULT NULL,
+  `b` varchar(21) DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+DROP TABLE t1;
+#
 # End of 10.1 tests
 #
diff --git a/mysql-test/t/func_hybrid_type.test b/mysql-test/t/func_hybrid_type.test
index 047e5f7..dd8a399 100644
--- a/mysql-test/t/func_hybrid_type.test
+++ b/mysql-test/t/func_hybrid_type.test
@@ -434,5 +434,28 @@ DROP TABLE t1;
 
 
 --echo #
+--echo # MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
+--echo #
+SELECT CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END;
+SELECT CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END;
+SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END;
+
+--disable_ps_protocol
+--enable_metadata
+SELECT COALESCE(COALESCE(NULL), 1.1) AS c0, IF(0, COALESCE(NULL), 1.1) AS c1;
+--disable_metadata
+--enable_ps_protocol
+
+
+--echo #
+--echo # MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements
+--echo #
+PREPARE stmt FROM "CREATE TABLE t1 AS SELECT CONCAT(COALESCE(?,1)) AS a, CONCAT(CASE WHEN TRUE THEN ? ELSE 1 END) AS b";
+SET @a=1;
+EXECUTE stmt USING @a,@a;
+SHOW CREATE TABLE t1;
+DROP TABLE t1;
+
+--echo #
 --echo # End of 10.1 tests
 --echo #
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index 2783a05..4819a7a 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -2246,50 +2246,6 @@ void Item_func_between::print(String *str, enum_query_type query_type)
 }
 
 
-void
-Item_func_case_abbreviation2::fix_length_and_dec2(Item **args)
-{
-  uint32 char_length;
-  set_handler_by_field_type(agg_field_type(args, 2, true));
-  maybe_null=args[0]->maybe_null || args[1]->maybe_null;
-  decimals= MY_MAX(args[0]->decimals, args[1]->decimals);
-  unsigned_flag= args[0]->unsigned_flag && args[1]->unsigned_flag;
-
-  if (Item_func_case_abbreviation2::result_type() == DECIMAL_RESULT ||
-      Item_func_case_abbreviation2::result_type() == INT_RESULT)
-  {
-    int len0= args[0]->max_char_length() - args[0]->decimals
-      - (args[0]->unsigned_flag ? 0 : 1);
-
-    int len1= args[1]->max_char_length() - args[1]->decimals
-      - (args[1]->unsigned_flag ? 0 : 1);
-
-    char_length= MY_MAX(len0, len1) + decimals + (unsigned_flag ? 0 : 1);
-  }
-  else
-    char_length= MY_MAX(args[0]->max_char_length(), args[1]->max_char_length());
-
-  switch (Item_func_case_abbreviation2::result_type()) {
-  case STRING_RESULT:
-    if (count_string_result_length(Item_func_case_abbreviation2::field_type(),
-                                   args, 2))
-      return;
-    break;
-  case DECIMAL_RESULT:
-  case REAL_RESULT:
-    break;
-  case INT_RESULT:
-    decimals= 0;
-    break;
-  case ROW_RESULT:
-  case TIME_RESULT:
-    DBUG_ASSERT(0);
-  }
-  fix_char_length(char_length);
-}
-
-
-
 uint Item_func_case_abbreviation2::decimal_precision2(Item **args) const
 {
   int arg0_int_part= args[0]->decimal_int_part();
@@ -3076,24 +3032,6 @@ bool Item_func_case::fix_fields(THD *thd, Item **ref)
 }
 
 
-void Item_func_case::agg_str_lengths(Item* arg)
-{
-  fix_char_length(MY_MAX(max_char_length(), arg->max_char_length()));
-  set_if_bigger(decimals, arg->decimals);
-  unsigned_flag= unsigned_flag && arg->unsigned_flag;
-}
-
-
-void Item_func_case::agg_num_lengths(Item *arg)
-{
-  uint len= my_decimal_length_to_precision(arg->max_length, arg->decimals,
-                                           arg->unsigned_flag) - arg->decimals;
-  set_if_bigger(max_length, len); 
-  set_if_bigger(decimals, arg->decimals);
-  unsigned_flag= unsigned_flag && arg->unsigned_flag; 
-}
-
-
 /**
   Check if (*place) and new_value points to different Items and call
   THD::change_item_tree() if needed.
@@ -3155,18 +3093,7 @@ void Item_func_case::fix_length_and_dec()
   }
   else
   {
-    collation.set_numeric();
-    max_length=0;
-    decimals=0;
-    unsigned_flag= TRUE;
-    for (uint i= 0; i < ncases; i+= 2)
-      agg_num_lengths(args[i + 1]);
-    if (else_expr_num != -1) 
-      agg_num_lengths(args[else_expr_num]);
-    max_length= my_decimal_precision_to_length_no_truncation(max_length +
-                                                             decimals,
-                                                             decimals,
-                                                             unsigned_flag);
+    fix_attributes(agg, nagg);
   }
   
   /*
@@ -3468,23 +3395,25 @@ my_decimal *Item_func_coalesce::decimal_op(my_decimal *decimal_value)
 }
 
 
-void Item_func_coalesce::fix_length_and_dec()
+void Item_hybrid_func::fix_attributes(Item **items, uint nitems)
 {
-  set_handler_by_field_type(agg_field_type(args, arg_count, true));
-  switch (Item_func_coalesce::result_type()) {
+  switch (Item_hybrid_func::result_type()) {
   case STRING_RESULT:
-    if (count_string_result_length(Item_func_coalesce::field_type(),
-                                   args, arg_count))
+    if (count_string_result_length(Item_hybrid_func::field_type(),
+                                   items, nitems))
       return;          
     break;
   case DECIMAL_RESULT:
-    count_decimal_length();
+    collation.set_numeric();
+    count_decimal_length(items, nitems);
     break;
   case REAL_RESULT:
-    count_real_length();
+    collation.set_numeric();
+    count_real_length(items, nitems);
     break;
   case INT_RESULT:
-    count_only_length(args, arg_count);
+    collation.set_numeric();
+    count_only_length(items, nitems);
     decimals= 0;
     break;
   case ROW_RESULT:
diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
index f17167b..0e17709 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -902,7 +902,11 @@ class Item_func_coalesce :public Item_func_hybrid_field_type
   String *str_op(String *);
   my_decimal *decimal_op(my_decimal *);
   bool date_op(MYSQL_TIME *ltime,uint fuzzydate);
-  void fix_length_and_dec();
+  void fix_length_and_dec()
+  {
+    set_handler_by_field_type(agg_field_type(args, arg_count, true));
+    fix_attributes(args, arg_count);
+  }
   const char *func_name() const { return "coalesce"; }
   table_map not_null_tables() const { return 0; }
 };
@@ -920,7 +924,11 @@ class Item_func_case_abbreviation2 :public Item_func_hybrid_field_type
     Item_func_hybrid_field_type(thd, a, b) { }
   Item_func_case_abbreviation2(THD *thd, Item *a, Item *b, Item *c):
     Item_func_hybrid_field_type(thd, a, b, c) { }
-  void fix_length_and_dec2(Item **args);
+  void fix_length_and_dec2(Item **args)
+  {
+    set_handler_by_field_type(agg_field_type(args, 2, true));
+    fix_attributes(args, 2);
+  }
   uint decimal_precision2(Item **args) const;
 };
 
@@ -1454,8 +1462,6 @@ class Item_func_case :public Item_func_hybrid_field_type
   Item *find_item(String *str);
   CHARSET_INFO *compare_collation() const { return cmp_collation.collation; }
   void cleanup();
-  void agg_str_lengths(Item *arg);
-  void agg_num_lengths(Item *arg);
   Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond);
 };
 
diff --git a/sql/item_func.cc b/sql/item_func.cc
index 52575a8..d26ad27 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -632,16 +632,25 @@ void Item_func::count_datetime_length(Item **item, uint nitems)
   result length/precision depends on argument ones.
 */
 
-void Item_func::count_decimal_length()
+void Item_func::count_decimal_length(Item **item, uint nitems)
 {
   int max_int_part= 0;
   decimals= 0;
   unsigned_flag= 1;
-  for (uint i=0 ; i < arg_count ; i++)
+  for (uint i=0 ; i < nitems ; i++)
   {
-    set_if_bigger(decimals, args[i]->decimals);
-    set_if_bigger(max_int_part, args[i]->decimal_int_part());
-    set_if_smaller(unsigned_flag, args[i]->unsigned_flag);
+    /*
+      Skip all explicit NULL derived arguments, because in a query like this:
+        SELECT COALESCE(COALESCE(NULL), 1.1)
+      the internal COALESCE has max_length==0 and decimals==31 (NOT_FIXED_DEC).
+      We want the result of this query to have decimals==1 rather than 31.
+    */
+    if (item[i]->field_type() != MYSQL_TYPE_NULL)
+    {
+      set_if_bigger(decimals, item[i]->decimals);
+      set_if_bigger(max_int_part, item[i]->decimal_int_part());
+    }
+    set_if_smaller(unsigned_flag, item[i]->unsigned_flag);
   }
   int precision= MY_MIN(max_int_part + decimals, DECIMAL_MAX_PRECISION);
   fix_char_length(my_decimal_precision_to_length_no_truncation(precision,
@@ -672,19 +681,20 @@ void Item_func::count_only_length(Item **item, uint nitems)
   result length/precision depends on argument ones.
 */
 
-void Item_func::count_real_length()
+void Item_func::count_real_length(Item **items, uint nitems)
 {
   uint32 length= 0;
   decimals= 0;
   max_length= 0;
-  for (uint i=0 ; i < arg_count ; i++)
+  unsigned_flag= false;
+  for (uint i=0 ; i < nitems ; i++)
   {
     if (decimals != NOT_FIXED_DEC)
     {
-      set_if_bigger(decimals, args[i]->decimals);
-      set_if_bigger(length, (args[i]->max_length - args[i]->decimals));
+      set_if_bigger(decimals, items[i]->decimals);
+      set_if_bigger(length, (items[i]->max_length - items[i]->decimals));
     }
-    set_if_bigger(max_length, args[i]->max_length);
+    set_if_bigger(max_length, items[i]->max_length);
   }
   if (decimals != NOT_FIXED_DEC)
   {
@@ -792,7 +802,7 @@ void Item_num_op::fix_length_and_dec(void)
   if (r0 == REAL_RESULT || r1 == REAL_RESULT ||
       r0 == STRING_RESULT || r1 ==STRING_RESULT)
   {
-    count_real_length();
+    count_real_length(args, arg_count);
     max_length= float_length(decimals);
     set_handler_by_result_type(REAL_RESULT);
   }
diff --git a/sql/item_func.h b/sql/item_func.h
index 6230550..851867d 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -149,8 +149,8 @@ class Item_func :public Item_func_or_sum
   void print_op(String *str, enum_query_type query_type);
   void print_args(String *str, uint from, enum_query_type query_type);
   void count_only_length(Item **item, uint nitems);
-  void count_real_length();
-  void count_decimal_length();
+  void count_real_length(Item **item, uint nitems);
+  void count_decimal_length(Item **item, uint nitems);
   inline bool get_arg0_date(MYSQL_TIME *ltime, ulonglong fuzzy_date)
   {
     return (null_value=args[0]->get_date_with_conversion(ltime, fuzzy_date));
@@ -387,6 +387,8 @@ class Item_real_func :public Item_func
 class Item_hybrid_func: public Item_func,
                         public Type_handler_hybrid_field_type
 {
+protected:
+  void fix_attributes(Item **item, uint nitems);
 public:
   Item_hybrid_func(THD *thd): Item_func(thd) { }
   Item_hybrid_func(THD *thd, Item *a):  Item_func(thd, a) { }