Fix for LP#612894 Some aggregate functions (such as MIN MAX) work incorrectly...

Fix for LP#612894 Some aggregate functions (such as MIN MAX) work incorrectly in subqueries after getting NULL value


mysql-test/r/group_by.result:
  Added test that showed problems that no_rows_in_results() didn't work for expressions
mysql-test/r/subselect4.result:
  Test case for LP#612894
mysql-test/t/group_by.test:
  Added test that showed problems that no_rows_in_results() didn't work for expressions
mysql-test/t/subselect4.test:
  Test case for LP#612894
sql/item.h:
  Added restore_to_before_no_rows_in_result()
  Added function processor for no_rows_in_results() and restore_to_before_no_rows_in_results() to ensure it works with functions
  Fix that above functions are handled by Item_ref()
sql/item_func.h:
  Ensure that no_rows_in_results() and restore_to_before_no_rows_in_result() are called for all function arguments
sql/item_sum.cc:
  Added restore_to_before_no_rows_in_result() to restore settings after Item_sum_hybrid::no_rows_in_result() was called.
  This is needed to handle the case where we have made 'make_const()' on the item in opt_sum(), but the item will be reused again in a sub query.
  Ignore multiple calls to no_rows_in_result() as Item_ref is calling it twice.
sql/item_sum.h:
  Added restore_to_before_no_rows_in_result();
sql/sql_select.cc:
  Added reset of no_rows_in_result() for JOIN::reinit()
sql/sql_select.h:
  Added marker if no_rows_in_result() is called.

mysql-test/r/group_by.result

@@ -1809,5 +1809,22 @@ SELECT MAX(t2.a) FROM t2 LEFT JOIN t1 ON t2.a = t1.a;
 MAX(t2.a)
 2
 DROP TABLE t1, t2;
+CREATE TABLE t1 (a int(11) NOT NULL);
+INSERT INTO t1 VALUES (1),(2);
+CREATE TABLE t2 (
+key_col int(11) NOT NULL,
+KEY (key_col)
+);
+INSERT INTO t2 VALUES (1),(2);
+select min(t2.key_col) from t1,t2 where t1.a=1;
+min(t2.key_col)
+1
+select min(t2.key_col) from t1,t2 where t1.a > 1000;
+min(t2.key_col)
+NULL
+select min(t2.key_col)+1 from t1,t2 where t1.a> 1000;
+min(t2.key_col)+1
+NULL
+drop table t1,t2;
 #
 # End of 5.1 tests

mysql-test/r/subselect4.result

@@ -59,3 +59,26 @@ FROM t3 WHERE 1 = 0 GROUP BY 1;
 (SELECT 1 FROM t1,t2 WHERE t2.b > t3.b)
 DROP TABLE t1,t2,t3;
 End of 5.0 tests.
+CREATE TABLE t1 (col_int_nokey int(11) NOT NULL, col_varchar_nokey varchar(1) NOT NULL) engine=myisam;
+INSERT INTO t1 VALUES (2,'s'),(0,'v'),(2,'s');
+CREATE TABLE t2 (
+pk int(11) NOT NULL AUTO_INCREMENT,
+`col_int_key` int(11) NOT NULL,
+col_varchar_key varchar(1) NOT NULL,
+PRIMARY KEY (pk),
+KEY `col_int_key` (`col_int_key`),
+KEY `col_varchar_key` (`col_varchar_key`)
+) ENGINE=MyISAM;
+INSERT INTO t2 VALUES (4,10,'g'), (5,20,'v');
+SELECT t1.col_int_nokey,(SELECT MIN( t2_a.col_int_key ) FROM t2 t2_a, t2 t2_b, t1 t1_a WHERE t1_a.col_varchar_nokey = t2_b.col_varchar_key and t1.col_int_nokey ) as sub FROM t1;
+col_int_nokey	sub
+2	10
+0	NULL
+2	10
+SELECT t1.col_int_nokey,(SELECT MIN( t2_a.col_int_key ) +1 FROM t2 t2_a, t2 t2_b, t1 t1_a WHERE t1_a.col_varchar_nokey = t2_b.col_varchar_key and t1.col_int_nokey ) as sub FROM t1;
+col_int_nokey	sub
+2	11
+0	NULL
+2	11
+DROP TABLE t1,t2;
+End of 5.1 tests.

mysql-test/t/group_by.test

@@ -1219,6 +1219,23 @@ EXPLAIN SELECT MAX(t2.a) FROM t2 LEFT JOIN t1 ON t2.a = t1.a;
 SELECT MAX(t2.a) FROM t2 LEFT JOIN t1 ON t2.a = t1.a;
 DROP TABLE t1, t2;
 
+#
+# min() returns wrong value when used in expression when there is no matching
+# rows
+#
+
+CREATE TABLE t1 (a int(11) NOT NULL);
+INSERT INTO t1 VALUES (1),(2);
+CREATE TABLE t2 (
+ key_col int(11) NOT NULL,
+ KEY (key_col)
+);
+INSERT INTO t2 VALUES (1),(2);
+
+select min(t2.key_col) from t1,t2 where t1.a=1;
+select min(t2.key_col) from t1,t2 where t1.a > 1000;
+select min(t2.key_col)+1 from t1,t2 where t1.a> 1000;
+drop table t1,t2;
 
 --echo #
 

mysql-test/t/subselect4.test

@@ -62,3 +62,29 @@ FROM t3 WHERE 1 = 0 GROUP BY 1;
 DROP TABLE t1,t2,t3;
 
 --echo End of 5.0 tests.
+
+#
+# Fix for LP#612894
+# Some aggregate functions (such as MIN MAX) work incorrectly in subqueries
+# after getting NULL value
+#
+
+CREATE TABLE t1 (col_int_nokey int(11) NOT NULL, col_varchar_nokey varchar(1) NOT NULL) engine=myisam;
+INSERT INTO t1 VALUES (2,'s'),(0,'v'),(2,'s');
+CREATE TABLE t2 (
+  pk int(11) NOT NULL AUTO_INCREMENT,
+ `col_int_key` int(11) NOT NULL,
+  col_varchar_key varchar(1) NOT NULL,
+  PRIMARY KEY (pk),
+  KEY `col_int_key` (`col_int_key`),
+  KEY `col_varchar_key` (`col_varchar_key`)
+) ENGINE=MyISAM;
+INSERT INTO t2 VALUES (4,10,'g'), (5,20,'v');
+
+SELECT t1.col_int_nokey,(SELECT MIN( t2_a.col_int_key ) FROM t2 t2_a, t2 t2_b, t1 t1_a WHERE t1_a.col_varchar_nokey = t2_b.col_varchar_key and t1.col_int_nokey ) as sub FROM t1;
+
+SELECT t1.col_int_nokey,(SELECT MIN( t2_a.col_int_key ) +1 FROM t2 t2_a, t2 t2_b, t1 t1_a WHERE t1_a.col_varchar_nokey = t2_b.col_varchar_key and t1.col_int_nokey ) as sub FROM t1;
+
+DROP TABLE t1,t2;
+
+--echo End of 5.1 tests.

sql/item.h

@@ -852,6 +852,7 @@ public:
     set value of aggregate function in case of no rows for grouping were found
   */
   virtual void no_rows_in_result() {}
+  virtual void restore_to_before_no_rows_in_result() {}
   virtual Item *copy_or_same(THD *thd) { return this; }
   virtual Item *copy_andor_structure(THD *thd) { return this; }
   virtual Item *real_item() { return this; }
@@ -908,6 +909,21 @@ public:
   virtual bool register_field_in_read_map(uchar *arg) { return 0; }
   virtual bool enumerate_field_refs_processor(uchar *arg) { return 0; }
   virtual bool mark_as_eliminated_processor(uchar *arg) { return 0; }
+
+  /* To call bool function for all arguments */
+  struct bool_func_call_args
+  {
+    Item *original_func_item;
+    void (Item::*bool_function)();
+  };
+  bool call_bool_func_processor(uchar *org_item)
+  {
+    bool_func_call_args *info= (bool_func_call_args*) org_item;
+    /* Avoid recursion, as walk also calls for original item */
+    if (info->original_func_item != this)
+      (this->*(info->bool_function))();
+    return FALSE;
+  }
   /*
     Check if a partition function is allowed
     SYNOPSIS
@@ -2321,6 +2337,14 @@ public:
     return (*ref)->walk(processor, walk_subquery, arg) ||
            (this->*processor)(arg);
   }
+  void no_rows_in_result()
+  {
+    (*ref)->no_rows_in_result();
+  }
+  void restore_to_before_no_rows_in_result()
+  {
+    (*ref)->restore_to_before_no_rows_in_result();
+  }
   virtual void print(String *str, enum_query_type query_type);
   bool result_as_longlong()
   {

sql/item_func.h

@@ -217,6 +217,21 @@ public:
   {
     return functype() == *(Functype *) arg;
   }
+
+  void no_rows_in_result()
+  {
+    bool_func_call_args info;
+    info.original_func_item= this;
+    info.bool_function= &Item::no_rows_in_result;
+    walk(&Item::call_bool_func_processor, FALSE, (uchar*) &info);
+  }
+  void restore_to_before_no_rows_in_result()
+  {
+    bool_func_call_args info;
+    info.original_func_item= this;
+    info.bool_function= &Item::restore_to_before_no_rows_in_result;
+    walk(&Item::call_bool_func_processor, FALSE, (uchar*) &info);
+  }
 };
 
 

sql/item_sum.cc

@@ -1638,8 +1638,22 @@ void Item_sum_hybrid::cleanup()
 
 void Item_sum_hybrid::no_rows_in_result()
 {
-  was_values= FALSE;
-  clear();
+  /* We may be called here twice in case of ref field in function */
+  if (was_values)
+  {
+    was_values= FALSE;
+    was_null_value= value->null_value;
+    clear();
+  }
+}
+
+void Item_sum_hybrid::restore_to_before_no_rows_in_result()
+{
+  if (!was_values)
+  {
+    was_values= TRUE;
+    null_value= value->null_value= was_null_value;
+  }
 }
 
 

sql/item_sum.h

@@ -496,7 +496,7 @@ public:
   enum Sumfunctype sum_func () const { return SUM_DISTINCT_FUNC; }
   void reset_field() {} // not used
   void update_field() {} // not used
-  virtual void no_rows_in_result() {}
+  void no_rows_in_result() {}
   void fix_length_and_dec();
   enum Item_result result_type () const { return val.traits->type(); }
   virtual void calculate_val_and_count();
@@ -845,6 +845,7 @@ protected:
   enum_field_types hybrid_field_type;
   int cmp_sign;
   bool was_values;  // Set if we have found at least one row (for max/min only)
+  bool was_null_value;
 
   public:
   Item_sum_hybrid(Item *item_par,int sign)
@@ -876,6 +877,7 @@ protected:
   void cleanup();
   bool any_value() { return was_values; }
   void no_rows_in_result();
+  void restore_to_before_no_rows_in_result();
   Field *create_tmp_field(bool group, TABLE *table,
 			  uint convert_blob_length);
 };

sql/sql_select.cc

@@ -1688,6 +1688,16 @@ JOIN::reinit()
       func->clear();
   }
 
+  if (no_rows_in_result_called)
+  {
+    /* Reset effect of possible no_rows_in_result() */
+    List_iterator_fast<Item> it(fields_list);
+    Item *item;
+
+    no_rows_in_result_called= 0;
+    while ((item= it++))
+      item->restore_to_before_no_rows_in_result();
+  }  
   DBUG_RETURN(0);
 }
 
@@ -12681,8 +12691,11 @@ end_send_group(JOIN *join, JOIN_TAB *join_tab __attribute__((unused)),
 	  {
             List_iterator_fast<Item> it(*join->fields);
             Item *item;
+            DBUG_PRINT("info", ("no matching rows"));
+
 	    /* No matching rows for group function */
 	    join->clear();
+            join->no_rows_in_result_called= 1;
 
             while ((item= it++))
               item->no_rows_in_result();

sql/sql_select.h

@@ -365,24 +365,26 @@ public:
     the number of rows in it may vary from one subquery execution to another.
   */
   bool no_const_tables; 
+  bool no_rows_in_result_called;
   
   /**
     Copy of this JOIN to be used with temporary tables.
 
-    tmp_join is used when the JOIN needs to be "reusable" (e.g. in a subquery
-    that gets re-executed several times) and we know will use temporary tables
-    for materialization. The materialization to a temporary table overwrites the
-    JOIN structure to point to the temporary table after the materialization is
-    done. This is where tmp_join is used : it's a copy of the JOIN before the
-    materialization and is used in restoring before re-execution by overwriting
-    the current JOIN structure with the saved copy.
-    Because of this we should pay extra care of not freeing up helper structures
-    that are referenced by the original contents of the JOIN. We can check for
-    this by making sure the "current" join is not the temporary copy, e.g.
-    !tmp_join || tmp_join != join
+    tmp_join is used when the JOIN needs to be "reusable" (e.g. in a
+    subquery that gets re-executed several times) and we know will use
+    temporary tables for materialization. The materialization to a
+    temporary table overwrites the JOIN structure to point to the
+    temporary table after the materialization is done. This is where
+    tmp_join is used : it's a copy of the JOIN before the
+    materialization and is used in restoring before re-execution by
+    overwriting the current JOIN structure with the saved copy.
+    Because of this we should pay extra care of not freeing up helper
+    structures that are referenced by the original contents of the
+    JOIN. We can check for this by making sure the "current" join is
+    not the temporary copy, e.g.  !tmp_join || tmp_join != join
  
-    We should free these sub-structures at JOIN::destroy() if the "current" join
-    has a copy is not that copy.
+    We should free these sub-structures at JOIN::destroy() if the
+    "current" join has a copy is not that copy.
   */
   JOIN *tmp_join;
   ROLLUP rollup;				///< Used with rollup
@@ -512,6 +514,7 @@ public:
     optimized= 0;
     cond_equal= 0;
     group_optimized_away= 0;
+    no_rows_in_result_called= 0;
 
     all_fields= fields_arg;
     if (&fields_list != &fields_arg)      /* Avoid valgrind-warning */