MDEV-8661 Wrong result for SELECT..WHERE a='a' AND a='a' COLLATE latin1_bin

MDEV-8679 Equal field propagation is not used for VARCHAR when it safely could

9 files changed, 198 insertions, 87 deletions

diff --git a/mysql-test/r/ctype_latin1.result b/mysql-test/r/ctype_latin1.result
index 727886091f7..df492800147 100644
--- a/mysql-test/r/ctype_latin1.result
+++ b/mysql-test/r/ctype_latin1.result
@@ -7915,3 +7915,41 @@ _latin1 0x7E	_latin1 X'7E'	_latin1 B'01111110'
 #
 # End of 10.0 tests
 #
+#
+# Start of 10.1 tests
+#
+#
+# MDEV-8661 Wrong result for SELECT..WHERE a='a' AND a='a' COLLATE latin1_bin
+#
+SET NAMES latin1;
+CREATE TABLE t1 (a CHAR(10));
+INSERT INTO t1 VALUES ('a'),('A');
+SELECT * FROM t1 WHERE a='a' AND a='a' COLLATE latin1_bin;
+a
+a
+DROP TABLE t1;
+CREATE TABLE t1 (a VARCHAR(10) CHARACTER SET latin1 COLLATE latin1_swedish_ci);
+INSERT INTO t1 VALUES ('a'),('A');
+SELECT * FROM t1 WHERE a='a' COLLATE latin1_bin AND a='A' COLLATE latin1_swedish_ci;
+a
+a
+DROP TABLE t1;
+#
+# MDEV-8679 Equal field propagation is not used for VARCHAR when it safely could
+#
+CREATE TABLE t1 (a VARCHAR(10));
+INSERT INTO t1 VALUES ('10'),('11'),('12');
+EXPLAIN EXTENDED SELECT * FROM t1 WHERE a='10' AND IF(a='10',1,0)=1;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
+1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	3	100.00	Using where
+Warnings:
+Note	1003	select `test`.`t1`.`a` AS `a` from `test`.`t1` where (`test`.`t1`.`a` = '10')
+EXPLAIN EXTENDED SELECT * FROM t1 WHERE a='10' AND CASE WHEN a='10' THEN 1 ELSE 0 END;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
+1	SIMPLE	t1	ALL	NULL	NULL	NULL	NULL	3	100.00	Using where
+Warnings:
+Note	1003	select `test`.`t1`.`a` AS `a` from `test`.`t1` where (`test`.`t1`.`a` = '10')
+DROP TABLE t1;
+#
+# End of 10.1 tests
+#
diff --git a/mysql-test/t/ctype_latin1.test b/mysql-test/t/ctype_latin1.test
index aeaad2cc026..b57aa8c442c 100644
--- a/mysql-test/t/ctype_latin1.test
+++ b/mysql-test/t/ctype_latin1.test
@@ -248,3 +248,34 @@ SELECT _latin1 0x7E, _latin1 X'7E', _latin1 B'01111110';
 --echo #
 --echo # End of 10.0 tests
 --echo #
+
+--echo #
+--echo # Start of 10.1 tests
+--echo #
+
+--echo #
+--echo # MDEV-8661 Wrong result for SELECT..WHERE a='a' AND a='a' COLLATE latin1_bin
+--echo #
+SET NAMES latin1;
+CREATE TABLE t1 (a CHAR(10));
+INSERT INTO t1 VALUES ('a'),('A');
+SELECT * FROM t1 WHERE a='a' AND a='a' COLLATE latin1_bin;
+DROP TABLE t1;
+
+CREATE TABLE t1 (a VARCHAR(10) CHARACTER SET latin1 COLLATE latin1_swedish_ci);
+INSERT INTO t1 VALUES ('a'),('A');
+SELECT * FROM t1 WHERE a='a' COLLATE latin1_bin AND a='A' COLLATE latin1_swedish_ci;
+DROP TABLE t1;
+
+--echo #
+--echo # MDEV-8679 Equal field propagation is not used for VARCHAR when it safely could
+--echo #
+CREATE TABLE t1 (a VARCHAR(10));
+INSERT INTO t1 VALUES ('10'),('11'),('12');
+EXPLAIN EXTENDED SELECT * FROM t1 WHERE a='10' AND IF(a='10',1,0)=1;
+EXPLAIN EXTENDED SELECT * FROM t1 WHERE a='10' AND CASE WHEN a='10' THEN 1 ELSE 0 END;
+DROP TABLE t1;
+
+--echo #
+--echo # End of 10.1 tests
+--echo #
diff --git a/sql/item.cc b/sql/item.cc
index 1639dbecc52..32cb824668e 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -5356,11 +5356,14 @@ Item_equal *Item_field::find_item_equal(COND_EQUAL *cond_equal)
     FALSE  otherwise
 */
 
-bool Item_field::subst_argument_checker(uchar **arg)
+bool Item_field::can_be_substituted_to_equal_item(const Context &ctx,
+                                                  const Item_equal *item_equal)
 {
-  return *arg &&
-         (*arg == (uchar *) Item::ANY_SUBST ||
-          result_type() != STRING_RESULT || 
+  if (cmp_context == STRING_RESULT &&
+      ctx.compare_collation() != item_equal->compare_collation())
+    return false;
+  return (ctx.subst_constraint() == ANY_SUBST ||
+          field->result_type() != STRING_RESULT ||
           (field->flags & BINARY_FLAG));
 }
 
@@ -5418,14 +5421,23 @@ static void convert_zerofill_number_to_string(THD *thd, Item **item, Field_num *
     - pointer to the field item, otherwise.
 */
 
-Item *Item_field::equal_fields_propagator(THD *thd, uchar *arg)
+Item *Item_field::propagate_equal_fields(THD *thd,
+                                         const Context &ctx,
+                                         COND_EQUAL *arg)
 {
   if (no_const_subst)
     return this;
   item_equal= find_item_equal((COND_EQUAL *) arg);
   Item *item= 0;
   if (item_equal)
+  {
+    if (!can_be_substituted_to_equal_item(ctx, item_equal))
+    {
+      item_equal= NULL;
+      return this;
+    }
     item= item_equal->get_const();
+  }
   /*
     Disable const propagation for items used in different comparison contexts.
     This must be done because, for example, Item_hex_string->val_int() is not
@@ -8124,43 +8136,6 @@ Item_equal *Item_direct_view_ref::find_item_equal(COND_EQUAL *cond_equal)
 
 
 /**
-  Check whether a reference to field item can be substituted for an equal item
-
-  @details
-  The function checks whether a substitution of a reference to field item for
-  an equal item is valid.
-
-  @param arg   *arg != NULL <-> the reference is in the context
-               where substitution for an equal item is valid
-
-  @note
-    See also the note for Item_field::subst_argument_checker
-
-  @retval
-    TRUE   substitution is valid
-  @retval
-    FALSE  otherwise
-*/
-bool Item_direct_view_ref::subst_argument_checker(uchar **arg)
-{
-  bool res= FALSE;
-  if (*arg)
-  { 
-    Item *item= real_item();
-    if (item->type() == FIELD_ITEM &&
-        (*arg == (uchar *) Item::ANY_SUBST || 
-         result_type() != STRING_RESULT ||
-         (((Item_field *) item)->field->flags & BINARY_FLAG)))
-      res= TRUE;
-  }
-  /* Block any substitution into the wrapped object */
-  if (*arg)
-    *arg= NULL; 
-  return res; 
-}
-
-
-/**
   Set a pointer to the multiple equality the view field reference belongs to
   (if any).
 
@@ -8180,7 +8155,7 @@ bool Item_direct_view_ref::subst_argument_checker(uchar **arg)
     of the compile method.
 
   @note 
-    The function calls Item_field::equal_fields_propagator for the field item
+    The function calls Item_field::propagate_equal_fields() for the field item
     this->real_item() to do the job. Then it takes the pointer to equal_item
     from this field item and assigns it to this->item_equal.
 
@@ -8189,12 +8164,14 @@ bool Item_direct_view_ref::subst_argument_checker(uchar **arg)
     - pointer to the field item, otherwise.
 */
 
-Item *Item_direct_view_ref::equal_fields_propagator(THD *thd, uchar *arg)
+Item *Item_direct_view_ref::propagate_equal_fields(THD *thd,
+                                                   const Context &ctx,
+                                                   COND_EQUAL *cond)
 {
   Item *field_item= real_item();
   if (field_item->type() != FIELD_ITEM)
     return this;
-  Item *item= field_item->equal_fields_propagator(thd, arg);
+  Item *item= field_item->propagate_equal_fields(thd, ctx, cond);
   set_item_equal(field_item->get_item_equal());
   field_item->set_item_equal(NULL);
   if (item != field_item)
diff --git a/sql/item.h b/sql/item.h
index 823dc79e87f..5db9b7851c7 100644
--- a/sql/item.h
+++ b/sql/item.h
@@ -1417,16 +1417,45 @@ public:
   */ 
   enum Subst_constraint 
   { 
-    NO_SUBST= 0,         /* No substitution for a field is allowed   */
     ANY_SUBST,           /* Any substitution for a field is allowed  */ 
     IDENTITY_SUBST       /* Substitution for a field is allowed if any two
                             different values of the field type are not equal */
   };
 
-  virtual bool subst_argument_checker(uchar **arg)
-  { 
-    return (*arg != NULL); 
-  }
+  /*
+    Item context attributes.
+    Comparison functions pass their attributes to propagate_equal_fields().
+    For exmple, for string comparison, the collation of the comparison
+    operation is important inside propagate_equal_fields().
+    TODO: We should move Item::cmp_context to from Item to Item::Context
+    eventually.
+  */
+  class Context
+  {
+    /*
+      Which type of propagation is allowed:
+      - SUBST_ANY (loose equality, according to the collation), or
+      - SUBST_IDENTITY (strict binary equality).
+    */
+    Subst_constraint m_subst_constraint;
+    /*
+      Collation of the comparison operation.
+      Important only when SUBST_ANY.
+    */
+    CHARSET_INFO *m_compare_collation;
+  public:
+    Context(Subst_constraint subst, CHARSET_INFO *cs)
+      :m_subst_constraint(subst), m_compare_collation(cs) { }
+    Context(Subst_constraint subst)
+      :m_subst_constraint(subst), m_compare_collation(&my_charset_bin) { }
+    Subst_constraint subst_constraint() const { return m_subst_constraint; }
+    CHARSET_INFO *compare_collation() const { return m_compare_collation; }
+  };
+
+  virtual Item* propagate_equal_fields(THD*, const Context &, COND_EQUAL *)
+  {
+    return this;
+  };
 
   /*
     @brief
@@ -1446,7 +1475,6 @@ public:
     return trace_unsupported_by_check_vcol_func_processor(full_name());
   }
 
-  virtual Item *equal_fields_propagator(THD *thd, uchar * arg) { return this; }
   virtual bool set_no_const_sub(uchar *arg) { return FALSE; }
   /* arg points to REPLACE_EQUAL_FIELD_ARG object */
   virtual Item *replace_equal_field(THD *thd, uchar *arg) { return this; }
@@ -1587,7 +1615,7 @@ public:
   }
   /**
     Check whether this and the given item has compatible comparison context.
-    Used by the equality propagation. See Item_field::equal_fields_propagator.
+    Used by the equality propagation. See Item_field::propagate_equal_fields()
 
     @return
       TRUE  if the context is the same
@@ -2403,8 +2431,9 @@ public:
   Item_equal *get_item_equal() { return item_equal; }
   void set_item_equal(Item_equal *item_eq) { item_equal= item_eq; }
   Item_equal *find_item_equal(COND_EQUAL *cond_equal);
-  bool subst_argument_checker(uchar **arg);
-  Item *equal_fields_propagator(THD *thd, uchar *arg);
+  bool can_be_substituted_to_equal_item(const Context &ctx,
+                                        const Item_equal *item);
+  Item* propagate_equal_fields(THD *, const Context &, COND_EQUAL *);
   bool set_no_const_sub(uchar *arg);
   Item *replace_equal_field(THD *thd, uchar *arg);
   inline uint32 max_disp_length() { return field->max_display_length(); }
@@ -3391,6 +3420,7 @@ protected:
     return false;
   }
   bool transform_args(THD *thd, Item_transformer transformer, uchar *arg);
+  void propagate_equal_fields(THD *, const Item::Context &, COND_EQUAL *);
 public:
   uint arg_count;
   Item_args(void)
@@ -3999,8 +4029,7 @@ public:
   Item_equal *get_item_equal() { return item_equal; }
   void set_item_equal(Item_equal *item_eq) { item_equal= item_eq; }
   Item_equal *find_item_equal(COND_EQUAL *cond_equal);
-  bool subst_argument_checker(uchar **arg);
-  Item *equal_fields_propagator(THD *thd, uchar *arg);
+  Item* propagate_equal_fields(THD *, const Context &, COND_EQUAL *);
   Item *replace_equal_field(THD *thd, uchar *arg);
   table_map used_tables() const;
   void update_used_tables();
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
index c2cf4e78271..78f0c21edbe 100644
--- a/sql/item_cmpfunc.cc
+++ b/sql/item_cmpfunc.cc
@@ -3115,7 +3115,7 @@ void Item_func_case::fix_length_and_dec()
     }
     /*
       Set cmp_context of all WHEN arguments. This prevents
-      Item_field::equal_fields_propagator() from transforming a
+      Item_field::propagate_equal_fields() from transforming a
       zerofill argument into a string constant. Such a change would
       require rebuilding cmp_items.
     */
@@ -4110,7 +4110,7 @@ void Item_func_in::fix_length_and_dec()
   }
   /*
     Set cmp_context of all arguments. This prevents
-    Item_field::equal_fields_propagator() from transforming a zerofill integer
+    Item_field::propagate_equal_fields() from transforming a zerofill integer
     argument into a string constant. Such a change would require rebuilding
     cmp_itmes.
    */
@@ -4561,6 +4561,29 @@ Item *Item_cond::compile(THD *thd, Item_analyzer analyzer, uchar **arg_p,
   return Item_func::transform(thd, transformer, arg_t);
 }
 
+
+Item *Item_cond::propagate_equal_fields(THD *thd,
+                                        const Context &ctx,
+                                        COND_EQUAL *cond)
+{
+  DBUG_ASSERT(!thd->stmt_arena->is_stmt_prepare());
+  DBUG_ASSERT(arg_count == 0);
+  List_iterator<Item> li(list);
+  Item *item;
+  while ((item= li++))
+  {
+    /*
+      The exact value of the second parameter to propagate_equal_fields()
+      is not important at this point. Item_func derivants will create and
+      pass their own context to the arguments.
+    */
+    Item *new_item= item->propagate_equal_fields(thd, ANY_SUBST, cond);
+    if (new_item && new_item != item)
+      thd->change_item_tree(li.ref(), new_item);
+  }
+  return this;
+}
+
 void Item_cond::traverse_cond(Cond_traverser traverser,
                               void *arg, traverse_order order)
 {
diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
index 0596bedad88..78afacece7e 100644
--- a/sql/item_cmpfunc.h
+++ b/sql/item_cmpfunc.h
@@ -369,9 +369,12 @@ public:
   }
   Item *neg_transformer(THD *thd);
   virtual Item *negated_item(THD *thd);
-  bool subst_argument_checker(uchar **arg)
+  Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond)
   {
-    return (*arg != NULL);     
+    Item_args::propagate_equal_fields(thd,
+                                      Context(ANY_SUBST, compare_collation()),
+                                      cond);
+    return this;
   }
   void fix_length_and_dec();
   int set_cmp_func()
@@ -418,9 +421,10 @@ public:
   { Item_func::print_op(str, query_type); }
   longlong val_int();
   Item *neg_transformer(THD *thd);
-  bool subst_argument_checker(uchar **arg)
+  Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond)
   {
-    return (*arg != NULL);     
+    Item_args::propagate_equal_fields(thd, ANY_SUBST, cond);
+    return this;
   }
 };
 
@@ -692,7 +696,13 @@ public:
     return this;
   }
   bool eq(const Item *item, bool binary_cmp) const;
-  bool subst_argument_checker(uchar **arg) { return TRUE; }
+  Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond)
+  {
+    Item_args::propagate_equal_fields(thd,
+                                      Context(ANY_SUBST, compare_collation()),
+                                      cond);
+    return this;
+  }
 };
 
 
@@ -1841,7 +1851,7 @@ public:
   void traverse_cond(Cond_traverser, void *arg, traverse_order order);
   void neg_arguments(THD *thd);
   enum_field_types field_type() const { return MYSQL_TYPE_LONGLONG; }
-  bool subst_argument_checker(uchar **arg) { return TRUE; }
+  Item* propagate_equal_fields(THD *, const Context &, COND_EQUAL *);
   Item *compile(THD *thd, Item_analyzer analyzer, uchar **arg_p,
                 Item_transformer transformer, uchar *arg_t);
   bool eval_not_null_tables(uchar *opt_arg);
diff --git a/sql/item_func.cc b/sql/item_func.cc
index 52f5a3709c2..fd63fa86224 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -415,6 +415,21 @@ Item *Item_func::compile(THD *thd, Item_analyzer analyzer, uchar **arg_p,
   return (this->*transformer)(thd, arg_t);
 }
 
+
+void Item_args::propagate_equal_fields(THD *thd,
+                                       const Item::Context &ctx,
+                                       COND_EQUAL *cond)
+{
+  Item **arg,**arg_end;
+  for (arg= args, arg_end= args + arg_count; arg != arg_end; arg++)
+  {
+    Item *new_item= (*arg)->propagate_equal_fields(thd, ctx, cond);
+    if (new_item && *arg != new_item)
+      thd->change_item_tree(arg, new_item);
+  }
+}
+
+
 /**
   See comments in Item_cond::split_sum_func()
 */
diff --git a/sql/item_func.h b/sql/item_func.h
index 244e5b05b1d..b9e431377b1 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -341,19 +341,15 @@ public:
     return FALSE;
   }
 
-  /*
-    By default only substitution for a field whose two different values
-    are never equal is allowed in the arguments of a function.
-    This is overruled for the direct arguments of comparison functions.
-  */ 
-  bool subst_argument_checker(uchar **arg) 
-  { 
-    if (*arg)
-    {
-      *arg= (uchar *) Item::IDENTITY_SUBST;
-      return TRUE;
-    }
-    return FALSE;
+  Item* propagate_equal_fields(THD *thd, const Context &ctx, COND_EQUAL *cond)
+  {
+    /*
+      By default only substitution for a field whose two different values
+      are never equal is allowed in the arguments of a function.
+      This is overruled for the direct arguments of comparison functions.
+    */
+    Item_args::propagate_equal_fields(thd, IDENTITY_SUBST, cond);
+    return this;
   }
 
   /*
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 4d78d7fbf58..daffa8a4cbe 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -13077,11 +13077,7 @@ COND *Item_func::build_equal_items(THD *thd, COND_EQUAL *inherited,
     as soon the field is not of a string type or the field reference is
     an argument of a comparison predicate. 
   */ 
-  uchar* is_subst_valid= (uchar *) Item::ANY_SUBST;
-  COND *cond= compile(thd, &Item::subst_argument_checker,
-                      &is_subst_valid, 
-                      &Item::equal_fields_propagator,
-                      (uchar *) inherited);
+  COND *cond= propagate_equal_fields(thd, ANY_SUBST, inherited);
   cond->update_used_tables();
   DBUG_ASSERT(cond == this);
   DBUG_ASSERT(!cond_equal_ref || !cond_equal_ref[0]);
@@ -14906,11 +14902,7 @@ void propagate_new_equalities(THD *thd, Item *cond,
   }
   else
   {
-    uchar* is_subst_valid= (uchar *) Item::ANY_SUBST;
-    cond= cond->compile(thd, &Item::subst_argument_checker,
-                        &is_subst_valid, 
-                        &Item::equal_fields_propagator,
-                        (uchar *) inherited);
+    cond= cond->propagate_equal_fields(thd, Item::ANY_SUBST, inherited);
     cond->update_used_tables();
   }          
 }