← Back to team overview

maria-developers team mailing list archive

Re: Please review MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler

 

Hello Alexey,

Thanks for your suggestions. An improved version is attached.
Please see comments inline:


On 02/07/2017 03:51 PM, Alexey Botchkov wrote:
> Hi, Alexander.
> 
> Basically i approve this patch. It seems in logic with the previous work
> in that area.
> Two comments though
>> -void Item_func_round::fix_length_and_dec()
>> +void Item_func_round::fix_length_and_dec_decimal(uint decimals_to_set)
>>  {
>> -  int      decimals_to_set;
>> -  longlong val1;
> ....
>> +  decimals= decimals_to_set;
>> +  int decimals_delta= args[0]->decimals - decimals_to_set;
>> +  int precision= args[0]->decimal_precision();
>> +  int length_increase= ((decimals_delta <= 0) || truncate) ? 0 : 1;
>> +  precision-= decimals_delta - length_increase;
>> +  max_length= my_decimal_precision_to_length_no_truncation(precision,
> .....
> 
> Please move declarations of the 'int decimals_delta' 'int precision' and
> 'int length_increase'
> to the beginning of the function. It does make reading easier, and our
> rules require it.
> I'd also replace two lines with one:
>> +  int precision= args[0]->decimal_precision();
>> +  precision-= decimals_delta - length_increase;
> to
>    precision= args[0]->decimal_precision() + length_increase -
> decimals_delta;

Done. Thanks. It now looks better.

> 
> +void Item_func_round::fix_arg_decimal()
> +{
>    if (!args[1]->const_item())
>    {
> 
> Since we started changing this code we can add some fixes to it.
> Firstly i think 'const_item()' should be replaced with the
> 'basic_const_item()' there.
> The later is faster and safer in case when the 'const item' is a
> complicated (yet constant) subquery.

As discussed on IRC, it can introduce a behavior change,
which is not desirable under terms of this task.

> Secondly it'd be nicer if we do the 'if (args[1]->const_item)' the first
> choice. (i mean not !args[1]->...)
>    but if you don't like that you can skip this suggestion :)

Done.

> 
> Best regards.
> HF.
> 
> 
> On Mon, Feb 6, 2017 at 11:26 AM, Alexander Barkov <bar@xxxxxxxxxxx
> <mailto:bar@xxxxxxxxxxx>> wrote:
> 
>     Hello Alexey,
> 
>     Can you please review a patch for MDEV-12001?
> 
>     Thanks!
> 
> 
commit 3f83801d8266fa465d296dbe528a5ea132945a84
Author: Alexander Barkov <bar@xxxxxxxxxxx>
Date:   Tue Feb 7 21:07:28 2017 +0400

    MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler
    
    This patch makes the following changes (according to the task description):
    - Adds Type_handler::Item_func_round_fix_length_and_dec().
    - Splits the code from Item_func_round::fix_length_and_dec() into new
      Item_func_round methods fix_arg_int(), fix_arg_decimal(), fix_arg_double().
    - Calls the new Item_func_round methods from the relevant implementations of
      Type_handler_xxx::Item_func_round_fix_length_and_dec().
    - Adds a new error message ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION
    - Makes ROUND() return the new error for GEOMETRY
    
    Additionally:
    - Inherits Item_func_round directly from Item_func_numhybrid as it
      uses nothing from Item_func_num1.
    - Fixes "MDEV-12000 ROUND(expr,const_expr_returning_NULL) creates DOUBLE(0,0)".
      Now if args[1] returns NULL, the data type is set to DOUBLE with
      NOT_FIXED_DEC decimals instead of 0 decimals.

diff --git a/mysql-test/r/func_math.result b/mysql-test/r/func_math.result
index f02776d..f26d1e4 100644
--- a/mysql-test/r/func_math.result
+++ b/mysql-test/r/func_math.result
@@ -817,3 +817,21 @@ select 0=0, 0=-0, 0.0= -0.0, 0.0 = -(0.0), 0.0E1=-0.0E1, 0.0E1=-(0.0E1);
 select CRC32(NULL), CRC32(''), CRC32('MySQL'), CRC32('mysql'), CRC32('01234567'), CRC32('012345678');
 CRC32(NULL)	CRC32('')	CRC32('MySQL')	CRC32('mysql')	CRC32('01234567')	CRC32('012345678')
 NULL	0	3259397556	2501908538	763378421	939184570
+#
+# Start of 10.3 tests
+#
+#
+# MDEV-12000 ROUND(expr,const_expr_returning_NULL) creates DOUBLE(0,0)
+#
+CREATE OR REPLACE TABLE t1 AS SELECT
+ROUND(10,NULL) AS c1,
+ROUND(10.1,NULL) AS c2,
+ROUND(10e0,NULL) AS c3;
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `c1` double DEFAULT NULL,
+  `c2` double DEFAULT NULL,
+  `c3` double DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+DROP TABLE t1;
diff --git a/mysql-test/r/gis.result b/mysql-test/r/gis.result
index 3da7682..1cf8f21 100644
--- a/mysql-test/r/gis.result
+++ b/mysql-test/r/gis.result
@@ -3711,5 +3711,14 @@ CASE a WHEN POINT(1,1) THEN "a" WHEN POINT(1,2) THEN "b" END
 DROP PROCEDURE p1;
 DROP PROCEDURE p2;
 #
+# MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler
+#
+CREATE TABLE t1 (a GEOMETRY);
+SELECT ROUND(a) FROM t1;
+ERROR HY000: Illegal parameter data type geometry for operation 'round'
+DROP TABLE t1;
+SELECT ROUND(POINT(1,1));
+ERROR HY000: Illegal parameter data type geometry for operation 'round'
+#
 # End of 10.3 tests
 #
diff --git a/mysql-test/t/func_math.test b/mysql-test/t/func_math.test
index 08349f0..dde6e2e 100644
--- a/mysql-test/t/func_math.test
+++ b/mysql-test/t/func_math.test
@@ -608,3 +608,19 @@ select 0=0, 0=-0, 0.0= -0.0, 0.0 = -(0.0), 0.0E1=-0.0E1, 0.0E1=-(0.0E1);
 --echo #
 
 select CRC32(NULL), CRC32(''), CRC32('MySQL'), CRC32('mysql'), CRC32('01234567'), CRC32('012345678');
+
+
+--echo #
+--echo # Start of 10.3 tests
+--echo #
+
+--echo #
+--echo # MDEV-12000 ROUND(expr,const_expr_returning_NULL) creates DOUBLE(0,0)
+--echo #
+
+CREATE OR REPLACE TABLE t1 AS SELECT
+  ROUND(10,NULL) AS c1,
+  ROUND(10.1,NULL) AS c2,
+  ROUND(10e0,NULL) AS c3;
+SHOW CREATE TABLE t1;
+DROP TABLE t1;
diff --git a/mysql-test/t/gis.test b/mysql-test/t/gis.test
index 7a1ddb4..7d6c3ba 100644
--- a/mysql-test/t/gis.test
+++ b/mysql-test/t/gis.test
@@ -1868,6 +1868,18 @@ CALL p1('CREATE TABLE t1 (a Point, b Point, c Point)');
 DROP PROCEDURE p1;
 DROP PROCEDURE p2;
 
+
+--echo #
+--echo # MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler
+--echo #
+
+CREATE TABLE t1 (a GEOMETRY);
+--error ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION
+SELECT ROUND(a) FROM t1;
+DROP TABLE t1;
+--error ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION
+SELECT ROUND(POINT(1,1));
+
 --echo #
 --echo # End of 10.3 tests
 --echo #
diff --git a/sql/item.h b/sql/item.h
index f9f7515..56bd8c5 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -842,6 +842,21 @@ class Item: public Value_source,
   */
   inline ulonglong val_uint() { return (ulonglong) val_int(); }
   /*
+    Adjust the result of val_int() to an unsigned number:
+    - NULL value is converted to 0. The caller can check "null_value"
+      to distinguish between 0 and NULL when necessary.
+    - Negative numbers are converted to 0.
+    - Positive numbers bigger than upper_bound are converted to upper_bound.
+    - Other numbers are returned as is.
+  */
+  ulonglong val_uint_from_val_int(ulonglong upper_bound)
+  {
+    longlong nr= val_int();
+    return (null_value || (nr < 0 && !unsigned_flag)) ? 0 :
+           (ulonglong) nr > upper_bound ? upper_bound : (ulonglong) nr;
+  }
+
+  /*
     Return string representation of this item object.
 
     SYNOPSIS
diff --git a/sql/item_func.cc b/sql/item_func.cc
index 4725808..ec91d7f 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -2347,85 +2347,92 @@ my_decimal *Item_func_floor::decimal_op(my_decimal *decimal_value)
 }
 
 
-void Item_func_round::fix_length_and_dec()
+void Item_func_round::fix_length_and_dec_decimal(uint decimals_to_set)
+{
+  int decimals_delta= args[0]->decimals - decimals_to_set;
+  int length_increase= (decimals_delta <= 0 || truncate) ? 0 : 1;
+  int precision= args[0]->decimal_precision() + length_increase -
+                                                decimals_delta;
+  DBUG_ASSERT(decimals_to_set <= DECIMAL_MAX_SCALE);
+  set_handler(&type_handler_newdecimal);
+  unsigned_flag= args[0]->unsigned_flag;
+  decimals= decimals_to_set;
+  max_length= my_decimal_precision_to_length_no_truncation(precision,
+                                                           decimals,
+                                                           unsigned_flag);
+}
+
+void Item_func_round::fix_length_and_dec_double(uint decimals_to_set)
 {
-  int      decimals_to_set;
-  longlong val1;
-  bool     val1_unsigned;
-  
+  set_handler(&type_handler_double);
   unsigned_flag= args[0]->unsigned_flag;
-  if (!args[1]->const_item())
+  decimals= decimals_to_set;
+  max_length= float_length(decimals_to_set);
+}
+
+
+void Item_func_round::fix_arg_decimal()
+{
+  if (args[1]->const_item())
   {
-    decimals= args[0]->decimals;
-    max_length= float_length(decimals);
-    if (args[0]->result_type() == DECIMAL_RESULT)
-    {
-      max_length++;
-      set_handler_by_result_type(DECIMAL_RESULT);
-    }
+    uint dec= (uint) args[1]->val_uint_from_val_int(DECIMAL_MAX_SCALE);
+    if (args[1]->null_value)
+      fix_length_and_dec_double(NOT_FIXED_DEC);
     else
-      set_handler_by_result_type(REAL_RESULT);
-    return;
+      fix_length_and_dec_decimal(dec);
   }
+  else
+  {
+    set_handler(&type_handler_newdecimal);
+    unsigned_flag= args[0]->unsigned_flag;
+    decimals= args[0]->decimals;
+    max_length= float_length(args[0]->decimals) + 1;
+  }
+}
 
-  val1= args[1]->val_int();
-  if ((null_value= args[1]->null_value))
-    return;
 
-  val1_unsigned= args[1]->unsigned_flag;
-  if (val1 < 0)
-    decimals_to_set= val1_unsigned ? INT_MAX : 0;
+void Item_func_round::fix_arg_double()
+{
+  if (args[1]->const_item())
+  {
+    uint dec= (uint) args[1]->val_uint_from_val_int(NOT_FIXED_DEC);
+    fix_length_and_dec_double(args[1]->null_value ? NOT_FIXED_DEC : dec);
+  }
   else
-    decimals_to_set= (val1 > INT_MAX) ? INT_MAX : (int) val1;
+    fix_length_and_dec_double(args[0]->decimals);
+}
+
 
-  if (args[0]->decimals == NOT_FIXED_DEC)
+void Item_func_round::fix_arg_int()
+{
+  if (args[1]->const_item())
   {
-    decimals= MY_MIN(decimals_to_set, NOT_FIXED_DEC);
-    max_length= float_length(decimals);
-    set_handler_by_result_type(REAL_RESULT);
-    return;
-  }
-  
-  switch (args[0]->result_type()) {
-  case REAL_RESULT:
-  case STRING_RESULT:
-    set_handler_by_result_type(REAL_RESULT);
-    decimals= MY_MIN(decimals_to_set, NOT_FIXED_DEC);
-    max_length= float_length(decimals);
-    break;
-  case INT_RESULT:
-    if ((!decimals_to_set && truncate) || (args[0]->decimal_precision() < DECIMAL_LONGLONG_DIGITS))
+    longlong val1= args[1]->val_int();
+    bool val1_is_negative= val1 < 0 && !args[1]->unsigned_flag;
+    uint decimals_to_set= val1_is_negative ?
+                          0 : (uint) MY_MIN(val1, DECIMAL_MAX_SCALE);
+    if (args[1]->null_value)
+      fix_length_and_dec_double(NOT_FIXED_DEC);
+    else if ((!decimals_to_set && truncate) ||
+             args[0]->decimal_precision() < DECIMAL_LONGLONG_DIGITS)
     {
-      int length_can_increase= MY_TEST(!truncate && (val1 < 0) &&
-                                       !val1_unsigned);
+      // Length can increase in some cases: ROUND(9,-1) -> 10
+      int length_can_increase= MY_TEST(!truncate && val1_is_negative);
       max_length= args[0]->max_length + length_can_increase;
-      /* Here we can keep INT_RESULT */
-      set_handler_by_result_type(INT_RESULT);
+      // Here we can keep INT_RESULT
+      set_handler(&type_handler_longlong);
+      unsigned_flag= args[0]->unsigned_flag;
       decimals= 0;
-      break;
     }
-    /* fall through */
-  case DECIMAL_RESULT:
-  {
-    set_handler_by_result_type(DECIMAL_RESULT);
-    decimals_to_set= MY_MIN(DECIMAL_MAX_SCALE, decimals_to_set);
-    int decimals_delta= args[0]->decimals - decimals_to_set;
-    int precision= args[0]->decimal_precision();
-    int length_increase= ((decimals_delta <= 0) || truncate) ? 0:1;
-
-    precision-= decimals_delta - length_increase;
-    decimals= MY_MIN(decimals_to_set, DECIMAL_MAX_SCALE);
-    max_length= my_decimal_precision_to_length_no_truncation(precision,
-                                                             decimals,
-                                                             unsigned_flag);
-    break;
-  }
-  case ROW_RESULT:
-  case TIME_RESULT:
-    DBUG_ASSERT(0); /* This result type isn't handled */
+    else
+      fix_length_and_dec_decimal(decimals_to_set);
   }
+  else
+    fix_length_and_dec_double(args[0]->decimals);
+
 }
 
+
 double my_double_round(double value, longlong dec, bool dec_unsigned,
                        bool truncate)
 {
diff --git a/sql/item_func.h b/sql/item_func.h
index fe0b766..42fd527 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -1202,17 +1202,25 @@ class Item_func_floor :public Item_func_int_val
 
 /* This handles round and truncate */
 
-class Item_func_round :public Item_func_num1
+class Item_func_round :public Item_func_numhybrid
 {
   bool truncate;
+  void fix_length_and_dec_decimal(uint decimals_to_set);
+  void fix_length_and_dec_double(uint decimals_to_set);
 public:
   Item_func_round(THD *thd, Item *a, Item *b, bool trunc_arg)
-    :Item_func_num1(thd, a, b), truncate(trunc_arg) {}
+    :Item_func_numhybrid(thd, a, b), truncate(trunc_arg) {}
   const char *func_name() const { return truncate ? "truncate" : "round"; }
   double real_op();
   longlong int_op();
   my_decimal *decimal_op(my_decimal *);
-  void fix_length_and_dec();
+  void fix_arg_decimal();
+  void fix_arg_int();
+  void fix_arg_double();
+  void fix_length_and_dec()
+  {
+    args[0]->type_handler()->Item_func_round_fix_length_and_dec(this);
+  }
   Item *get_copy(THD *thd, MEM_ROOT *mem_root)
   { return get_item_copy<Item_func_round>(thd, mem_root, this); }
 };
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
index a075764..c0796b2 100644
--- a/sql/share/errmsg-utf8.txt
+++ b/sql/share/errmsg-utf8.txt
@@ -7450,3 +7450,5 @@ ER_JSON_PATH_EMPTY
         eng "Path expression '$' is not allowed in argument %d to function '%s'."
 ER_ILLEGAL_PARAMETER_DATA_TYPES2_FOR_OPERATION
         eng "Illegal parameter data types %s and %s for operation '%s'"
+ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION
+        eng "Illegal parameter data type %s for operation '%s'"
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index d165470..6de1af2 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -26,7 +26,6 @@ static Type_handler_long        type_handler_long;
 static Type_handler_int24       type_handler_int24;
 static Type_handler_year        type_handler_year;
 static Type_handler_float       type_handler_float;
-static Type_handler_double      type_handler_double;
 static Type_handler_time        type_handler_time;
 static Type_handler_time2       type_handler_time2;
 static Type_handler_date        type_handler_date;
@@ -51,6 +50,7 @@ Type_handler_null        type_handler_null;
 Type_handler_row         type_handler_row;
 Type_handler_varchar     type_handler_varchar;
 Type_handler_longlong    type_handler_longlong;
+Type_handler_double      type_handler_double;
 Type_handler_newdecimal  type_handler_newdecimal;
 Type_handler_datetime    type_handler_datetime;
 Type_handler_bit         type_handler_bit;
@@ -2155,3 +2155,63 @@ String *Type_handler_timestamp_common::
 
 
 /***************************************************************************/
+
+bool Type_handler_row::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  DBUG_ASSERT(0);
+  return false;
+}
+
+
+bool Type_handler_int_result::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  item->fix_arg_int();
+  return false;
+}
+
+
+bool Type_handler_real_result::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  item->fix_arg_double();
+  return false;
+}
+
+
+bool Type_handler_decimal_result::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  item->fix_arg_decimal();
+  return false;
+}
+
+
+bool Type_handler_temporal_result::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  item->fix_arg_double();
+  return false;
+}
+
+
+bool Type_handler_string_result::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  item->fix_arg_double();
+  return false;
+}
+
+
+#ifdef HAVE_SPATIAL
+bool Type_handler_geometry::
+       Item_func_round_fix_length_and_dec(Item_func_round *item) const
+{
+  my_error(ER_ILLEGAL_PARAMETER_DATA_TYPE_FOR_OPERATION, MYF(0),
+           type_handler_geometry.name().ptr(), item->func_name());
+  return false;
+}
+#endif
+
+/***************************************************************************/
diff --git a/sql/sql_type.h b/sql/sql_type.h
index 79cdba9..45ffef5 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -35,6 +35,7 @@ class Item_func_hybrid_field_type;
 class Item_bool_func2;
 class Item_func_between;
 class Item_func_in;
+class Item_func_round;
 class cmp_item;
 class in_vector;
 class Type_std_attributes;
@@ -443,6 +444,9 @@ class Type_handler
   virtual bool
   Item_func_in_fix_comparator_compatible_types(THD *thd, Item_func_in *)
                                                                const= 0;
+
+  virtual bool
+  Item_func_round_fix_length_and_dec(Item_func_round *round) const= 0;
 };
 
 
@@ -594,6 +598,7 @@ class Type_handler_row: public Type_handler
   in_vector *make_in_vector(THD *thd, const Item_func_in *f, uint nargs) const;
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -662,6 +667,7 @@ class Type_handler_real_result: public Type_handler_numeric
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
 
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -704,6 +710,7 @@ class Type_handler_decimal_result: public Type_handler_numeric
   in_vector *make_in_vector(THD *, const Item_func_in *, uint nargs) const;
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -745,6 +752,7 @@ class Type_handler_int_result: public Type_handler_numeric
   in_vector *make_in_vector(THD *, const Item_func_in *, uint nargs) const;
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -790,6 +798,7 @@ class Type_handler_temporal_result: public Type_handler
   longlong Item_func_between_val_int(Item_func_between *func) const;
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -849,6 +858,7 @@ class Type_handler_string_result: public Type_handler
   in_vector *make_in_vector(THD *, const Item_func_in *, uint nargs) const;
   bool Item_func_in_fix_comparator_compatible_types(THD *thd,
                                                     Item_func_in *) const;
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 
 
@@ -1263,6 +1273,7 @@ class Type_handler_geometry: public Type_handler_string_result
   {
     return false;
   }
+  bool Item_func_round_fix_length_and_dec(Item_func_round *) const;
 };
 #endif
 
@@ -1381,6 +1392,7 @@ extern Type_handler_row   type_handler_row;
 extern Type_handler_null  type_handler_null;
 extern Type_handler_varchar type_handler_varchar;
 extern Type_handler_longlong type_handler_longlong;
+extern Type_handler_double type_handler_double;
 extern Type_handler_newdecimal type_handler_newdecimal;
 extern Type_handler_datetime type_handler_datetime;
 extern Type_handler_longlong type_handler_longlong;

References