maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Forgot to attach the patch in the previous letter.


On 12/10/2016 08:17 PM, Alexander Barkov wrote:
> Hello Vicențiu,
> 
> can you please review a patch for MDEV-9522 ?
> 
> Thanks!
> 

diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
index bf7f8fe..6088308 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -391,7 +391,7 @@ class Item_bool_func2 :public Item_bool_func
     Specifies which result type the function uses to compare its arguments.
     This method is used in equal field propagation.
   */
-  virtual Item_result compare_type() const
+  virtual const Type_handler *compare_type_handler() const
   {
     /*
       Have STRING_RESULT by default, which means the function compares
@@ -399,7 +399,7 @@ class Item_bool_func2 :public Item_bool_func
       and for Item_func_spatial_rel.
       Note, Item_bool_rowready_func2 overrides this default behaviour.
     */
-    return STRING_RESULT;
+    return &type_handler_varchar;
   }
   SEL_TREE *get_mm_tree(RANGE_OPT_PARAM *param, Item **cond_ptr)
   {
@@ -506,7 +506,10 @@ class Item_bool_rowready_func2 :public Item_bool_func2_with_rev
     return cmp.set_cmp_func(this, tmp_arg, tmp_arg + 1, true);
   }
   CHARSET_INFO *compare_collation() const { return cmp.compare_collation(); }
-  Item_result compare_type() const { return cmp.compare_type(); }
+  const Type_handler *compare_type_handler() const
+  {
+    return cmp.compare_type_handler();
+  }
   Arg_comparator *get_comparator() { return &cmp; }
   void cleanup()
   {
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 168e0af..d6290a7 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -12926,7 +12926,7 @@ bool Item_func_eq::check_equality(THD *thd, COND_EQUAL *cond_equal,
   }
   return check_simple_equality(thd,
                                Context(ANY_SUBST,
-                                       compare_type(),
+                                       compare_type_handler()->cmp_type(),
                                        compare_collation()),
                                left_item, right_item, cond_equal);
 }
@@ -13994,71 +13994,11 @@ can_change_cond_ref_to_const(Item_bool_func2 *target,
                              Item_bool_func2 *source,
                              Item *source_expr, Item *source_const)
 {
-  if (!target_expr->eq(source_expr,0) ||
-       target_value == source_const ||
-       target->compare_type() != source->compare_type())
-    return false;
-  if (target->compare_type() == STRING_RESULT)
-  {
-    /*
-      In this example:
-        SET NAMES utf8 COLLATE utf8_german2_ci;
-        DROP TABLE IF EXISTS t1;
-        CREATE TABLE t1 (a CHAR(10) CHARACTER SET utf8);
-        INSERT INTO t1 VALUES ('o-umlaut'),('oe');
-        SELECT * FROM t1 WHERE a='oe' COLLATE utf8_german2_ci AND a='oe';
-
-      the query should return only the row with 'oe'.
-      It should not return 'o-umlaut', because 'o-umlaut' does not match
-      the right part of the condition: a='oe'
-      ('o-umlaut' is not equal to 'oe' in utf8_general_ci,
-       which is the collation of the field "a").
-
-      If we change the right part from:
-         ... AND a='oe'
-      to
-         ... AND 'oe' COLLATE utf8_german2_ci='oe'
-      it will be evalulated to TRUE and removed from the condition,
-      so the overall query will be simplified to:
-
-        SELECT * FROM t1 WHERE a='oe' COLLATE utf8_german2_ci;
-
-      which will erroneously start to return both 'oe' and 'o-umlaut'.
-      So changing "expr" to "const" is not possible if the effective
-      collations of "target" and "source" are not exactly the same.
-
-      Note, the code before the fix for MDEV-7152 only checked that
-      collations of "source_const" and "target_value" are the same.
-      This was not enough, as the bug report demonstrated.
-    */
-    return
-      target->compare_collation() == source->compare_collation() &&
-      target_value->collation.collation == source_const->collation.collation;
-  }
-  if (target->compare_type() == TIME_RESULT)
-  {
-    if (target_value->cmp_type() != TIME_RESULT)
-    {
-      /*
-        Can't rewrite:
-          WHERE COALESCE(time_column)='00:00:00'
-            AND COALESCE(time_column)=DATE'2015-09-11'
-        to
-          WHERE DATE'2015-09-11'='00:00:00'
-            AND COALESCE(time_column)=DATE'2015-09-11'
-        because the left part will erroneously try to parse '00:00:00'
-        as DATE, not as TIME.
-
-        TODO: It could still be rewritten to:
-          WHERE DATE'2015-09-11'=TIME'00:00:00'
-            AND COALESCE(time_column)=DATE'2015-09-11'
-        i.e. we need to replace both target_expr and target_value
-        at the same time. This is not supported yet.
-      */
-      return false;
-    }
-  }
-  return true; // Non-string comparison
+  return target_expr->eq(source_expr,0) &&
+         target_value != source_const &&
+         target->compare_type_handler()->
+           can_change_cond_ref_to_const(target, target_expr, target_value,
+                                        source, source_expr, source_const);
 }
 
 
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index 1e6ed4d..47b7853 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -773,6 +773,95 @@ bool Type_handler_temporal_result::set_comparator_func(Arg_comparator *cmp) cons
 
 /*************************************************************************/
 
+bool Type_handler_temporal_result::
+       can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                    Item *target_expr, Item *target_value,
+                                    Item_bool_func2 *source,
+                                    Item *source_expr, Item *source_const)
+                                    const
+{
+  if (source->compare_type_handler()->cmp_type() != TIME_RESULT)
+    return false;
+
+  /*
+    Can't rewrite:
+      WHERE COALESCE(time_column)='00:00:00'
+        AND COALESCE(time_column)=DATE'2015-09-11'
+    to
+      WHERE DATE'2015-09-11'='00:00:00'
+        AND COALESCE(time_column)=DATE'2015-09-11'
+    because the left part will erroneously try to parse '00:00:00'
+    as DATE, not as TIME.
+
+    TODO: It could still be rewritten to:
+      WHERE DATE'2015-09-11'=TIME'00:00:00'
+        AND COALESCE(time_column)=DATE'2015-09-11'
+    i.e. we need to replace both target_expr and target_value
+    at the same time. This is not supported yet.
+  */
+  return target_value->cmp_type() == TIME_RESULT;
+}
+
+
+bool Type_handler_string_result::
+       can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                    Item *target_expr, Item *target_value,
+                                    Item_bool_func2 *source,
+                                    Item *source_expr, Item *source_const)
+                                    const
+{
+  if (source->compare_type_handler()->cmp_type() != STRING_RESULT)
+    return false;
+  /*
+    In this example:
+      SET NAMES utf8 COLLATE utf8_german2_ci;
+      DROP TABLE IF EXISTS t1;
+      CREATE TABLE t1 (a CHAR(10) CHARACTER SET utf8);
+      INSERT INTO t1 VALUES ('o-umlaut'),('oe');
+      SELECT * FROM t1 WHERE a='oe' COLLATE utf8_german2_ci AND a='oe';
+
+    the query should return only the row with 'oe'.
+    It should not return 'o-umlaut', because 'o-umlaut' does not match
+    the right part of the condition: a='oe'
+    ('o-umlaut' is not equal to 'oe' in utf8_general_ci,
+     which is the collation of the field "a").
+
+    If we change the right part from:
+       ... AND a='oe'
+    to
+       ... AND 'oe' COLLATE utf8_german2_ci='oe'
+    it will be evalulated to TRUE and removed from the condition,
+    so the overall query will be simplified to:
+
+      SELECT * FROM t1 WHERE a='oe' COLLATE utf8_german2_ci;
+
+    which will erroneously start to return both 'oe' and 'o-umlaut'.
+    So changing "expr" to "const" is not possible if the effective
+    collations of "target" and "source" are not exactly the same.
+
+    Note, the code before the fix for MDEV-7152 only checked that
+    collations of "source_const" and "target_value" are the same.
+    This was not enough, as the bug report demonstrated.
+  */
+  return
+    target->compare_collation() == source->compare_collation() &&
+    target_value->collation.collation == source_const->collation.collation;
+}
+
+
+bool Type_handler_numeric::
+       can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                    Item *target_expr, Item *target_value,
+                                    Item_bool_func2 *source,
+                                    Item *source_expr, Item *source_const)
+                                    const
+{
+  return target->compare_type_handler() == source->compare_type_handler();
+}
+
+
+/*************************************************************************/
+
 Item_cache *
 Type_handler_row::Item_get_cache(THD *thd, const Item *item) const
 {
diff --git a/sql/sql_type.h b/sql/sql_type.h
index 9086e60..5a0bcf4 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -29,6 +29,7 @@ class Item_cache;
 class Item_sum_hybrid;
 class Item_func_hex;
 class Item_func_hybrid_field_type;
+class Item_bool_func2;
 class Item_func_between;
 class Type_std_attributes;
 class Sort_param;
@@ -291,6 +292,31 @@ class Type_handler
 
   virtual int Item_save_in_field(Item *item, Field *field,
                                  bool no_conversions) const= 0;
+  /**
+    Check if
+      WHERE expr=value AND expr=const
+    can be rewritten as:
+      WHERE const=value AND expr=const
+
+    "this" is the comparison handler that is used by "target".
+
+    @param target       - the target operator whose "expr" argument will be
+                          replaced to "const".
+    @param target_expr  - the target's "expr" which will be replaced to "const".
+    @param target_value - the target's second argument, it will remain unchanged.
+    @param source       - the equality expression ("=" or "<=>") that
+                          can be used to rewrite the "target" part
+                          (under certain conditions, see the code).
+    @param source_expr  - the source's "expr". It should be exactly equal to 
+                          the target's "expr" to make condition rewrite possible.
+    @param source_const - the source's "const" argument, it will be inserted
+                          into "target" instead of "expr".
+  */
+  virtual bool
+  can_change_cond_ref_to_const(Item_bool_func2 *target,
+                               Item *target_expr, Item *target_value,
+                               Item_bool_func2 *source,
+                               Item *source_expr, Item *source_const) const= 0;
   virtual Item_cache *Item_get_cache(THD *thd, const Item *item) const= 0;
   virtual bool set_comparator_func(Arg_comparator *cmp) const= 0;
   virtual bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *) const= 0;
@@ -368,6 +394,14 @@ class Type_handler_row: public Type_handler
     DBUG_ASSERT(0);
     return 1;
   }
+  bool can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                   Item *target_expr, Item *target_value,
+                                   Item_bool_func2 *source,
+                                   Item *source_expr, Item *source_const) const
+  {
+    DBUG_ASSERT(0);
+    return false;
+  }
   Item_cache *Item_get_cache(THD *thd, const Item *item) const;
   bool set_comparator_func(Arg_comparator *cmp) const;
   bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const
@@ -428,6 +462,10 @@ class Type_handler_numeric: public Type_handler
                                                   const;
 public:
   virtual ~Type_handler_numeric() { }
+  bool can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                   Item *target_expr, Item *target_value,
+                                   Item_bool_func2 *source,
+                                   Item *source_expr, Item *source_const) const;
 };
 
 
@@ -542,7 +580,10 @@ class Type_handler_temporal_result: public Type_handler
   void sortlength(THD *thd,
                   const Type_std_attributes *item,
                   SORT_FIELD_ATTR *attr) const;
-  Item_cache *Item_get_cache(THD *thd, const Item *item) const;
+  bool can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                   Item *target_expr, Item *target_value,
+                                   Item_bool_func2 *source,
+                                   Item *source_expr, Item *source_const) const;  Item_cache *Item_get_cache(THD *thd, const Item *item) const;
   bool set_comparator_func(Arg_comparator *cmp) const;
   bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
   String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;
@@ -577,7 +618,10 @@ class Type_handler_string_result: public Type_handler
                   const Type_std_attributes *item,
                   SORT_FIELD_ATTR *attr) const;
   int Item_save_in_field(Item *item, Field *field, bool no_conversions) const;
-  Item_cache *Item_get_cache(THD *thd, const Item *item) const;
+  bool can_change_cond_ref_to_const(Item_bool_func2 *target,
+                                   Item *target_expr, Item *target_value,
+                                   Item_bool_func2 *source,
+                                   Item *source_expr, Item *source_const) const;  Item_cache *Item_get_cache(THD *thd, const Item *item) const;
   bool set_comparator_func(Arg_comparator *cmp) const;
   bool Item_sum_hybrid_fix_length_and_dec(Item_sum_hybrid *func) const;
   String *Item_func_hex_val_str_ascii(Item_func_hex *item, String *str) const;