Bug#49771: Incorrect MIN/MAX for date/time values.

This bug is a design flaw of the fix for the bug#33546. It assumed that an
item can be used only in one comparison context, but actually it isn't the
case. Item_cache_datetime is used to store result for MIX/MAX aggregate
functions. Because Arg_comparator always compares datetime values as INTs when
possible the Item_cache_datetime most time caches only INT value. But
since all datetime values has STRING result type MIN/MAX functions are asked
for a STRING value when the result is being sent to a client. The
Item_cache_datetime was designed to avoid conversions and get INT/STRING
values from an underlying item, but at the moment the values is asked
underlying item doesn't hold it anymore thus wrong result is returned.
Beside that MIN/MAX aggregate functions was wrongly initializing cached result
and this led to a wrong result.

The Item::has_compatible_context helper function is added. It checks whether
this and given items has the same comparison context or can be compared as
DATETIME values by Arg_comparator. The equality propagation optimization is
adjusted to take into account that items which being compared as DATETIME
can have different comparison contexts.
The Item_cache_datetime now converts cached INT value to a correct STRING
DATETIME value by means of number_to_datetime & my_TIME_to_str functions.
The Arg_comparator::set_cmp_context_for_datetime helper function is added. 
It sets comparison context of items being compared as DATETIMEs to INT if
items will be compared as longlong.
The Item_sum_hybrid::setup function now correctly initializes its result
value.
In order to avoid unnecessary conversions Item_sum_hybrid now states that it
can provide correct longlong value if the item being aggregated can do it
too.

mysql-test/r/group_by.result:
  Added a test case for the bug#49771.
sql/item.cc:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  The equality propagation mechanism is adjusted to take into account that
  items which being compared as DATETIME can have different comparison
  contexts.
  The Item_cache_datetime now converts cached INT value to a correct STRING
  DATETIME/TIME value.
sql/item.h:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  The Item::has_compatible_context helper function is added. It checks whether
  this and given items has the same comparison context or can be compared as
  DATETIME values by Arg_comparator.
  Added Item_cache::clear helper function.
sql/item_cmpfunc.cc:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  The Arg_comparator::set_cmp_func now sets the correct comparison context
  for items being compared as DATETIME values.
sql/item_cmpfunc.h:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  The Arg_comparator::set_cmp_context_for_datetime helper function is added. 
  It sets comparison context of items being compared as DATETIMEs to INT if
  items will be compared as longlong.
sql/item_sum.cc:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  The Item_sum_hybrid::setup function now correctly initializes its result
  value.
sql/item_sum.h:
  Bug#49771: Incorrect MIN/MAX for date/time values.
  In order to avoid unnecessary conversions Item_sum_hybrid now states that it
  can provide correct longlong value if the item being aggregated can do it
  too.

mysql-test/r/group_by.result

@@ -1812,3 +1812,32 @@ MAX(t2.a)
 DROP TABLE t1, t2;
 #
 # End of 5.1 tests
+#
+# Bug#49771: Incorrect MIN (date) when minimum value is 0000-00-00
+#
+CREATE TABLE t1 (f1 int, f2 DATE);
+INSERT INTO t1 VALUES (1,'2004-04-19'), (1,'0000-00-00'), (1,'2004-04-18'),
+(2,'2004-05-19'), (2,'0001-01-01'), (3,'2004-04-10');
+SELECT MIN(f2),MAX(f2) FROM t1;
+MIN(f2)	MAX(f2)
+0000-00-00	2004-05-19
+SELECT f1,MIN(f2),MAX(f2) FROM t1 GROUP BY 1;
+f1	MIN(f2)	MAX(f2)
+1	0000-00-00	2004-04-19
+2	0001-01-01	2004-05-19
+3	2004-04-10	2004-04-10
+DROP TABLE t1;
+CREATE TABLE t1 ( f1 int, f2 time);
+INSERT INTO t1 VALUES (1,'01:27:35'), (1,'06:11:01'), (2,'19:53:05'),
+(2,'21:44:25'), (3,'10:55:12'), (3,'05:45:11'), (4,'00:25:00');
+SELECT MIN(f2),MAX(f2) FROM t1;
+MIN(f2)	MAX(f2)
+00:25:00	21:44:25
+SELECT f1,MIN(f2),MAX(f2) FROM t1 GROUP BY 1;
+f1	MIN(f2)	MAX(f2)
+1	01:27:35	06:11:01
+2	19:53:05	21:44:25
+3	05:45:11	10:55:12
+4	00:25:00	00:25:00
+DROP TABLE t1;
+#End of test#49771

mysql-test/t/group_by.test

@@ -1224,3 +1224,26 @@ DROP TABLE t1, t2;
 --echo #
 
 --echo # End of 5.1 tests
+
+--echo #
+--echo # Bug#49771: Incorrect MIN (date) when minimum value is 0000-00-00
+--echo #
+CREATE TABLE t1 (f1 int, f2 DATE);
+
+INSERT INTO t1 VALUES (1,'2004-04-19'), (1,'0000-00-00'), (1,'2004-04-18'),
+(2,'2004-05-19'), (2,'0001-01-01'), (3,'2004-04-10');
+
+SELECT MIN(f2),MAX(f2) FROM t1;
+SELECT f1,MIN(f2),MAX(f2) FROM t1 GROUP BY 1;
+
+DROP TABLE t1;
+
+CREATE TABLE t1 ( f1 int, f2 time);
+INSERT INTO t1 VALUES (1,'01:27:35'), (1,'06:11:01'), (2,'19:53:05'),
+(2,'21:44:25'), (3,'10:55:12'), (3,'05:45:11'), (4,'00:25:00');
+
+SELECT MIN(f2),MAX(f2) FROM t1;
+SELECT f1,MIN(f2),MAX(f2) FROM t1 GROUP BY 1;
+
+DROP TABLE t1;
+--echo #End of test#49771

sql/item.cc

@@ -4914,11 +4914,8 @@ Item *Item_field::equal_fields_propagator(uchar *arg)
     e.g. <bin_col> = <int_col> AND <bin_col> = <hex_string>) since
     Items don't know the context they are in and there are functions like 
     IF (<hex_string>, 'yes', 'no').
-    The same problem occurs when comparing a DATE/TIME field with a
-    DATE/TIME represented as an int and as a string.
   */
-  if (!item ||
-      (cmp_context != (Item_result)-1 && item->cmp_context != cmp_context))
+  if (!item || !has_compatible_context(item))
     item= this;
   else if (field && (field->flags & ZEROFILL_FLAG) && IS_NUM(field->type()))
   {
@@ -4982,8 +4979,7 @@ Item *Item_field::replace_equal_field(uchar *arg)
     Item *const_item= item_equal->get_const();
     if (const_item)
     {
-      if (cmp_context != (Item_result)-1 &&
-          const_item->cmp_context != cmp_context)
+      if (!has_compatible_context(const_item))
         return this;
       return const_item;
     }
@@ -5053,21 +5049,6 @@ enum_field_types Item::field_type() const
 }
 
 
-bool Item::is_datetime()
-{
-  switch (field_type())
-  {
-    case MYSQL_TYPE_DATE:
-    case MYSQL_TYPE_DATETIME:
-    case MYSQL_TYPE_TIMESTAMP:
-      return TRUE;
-    default:
-      break;
-  }
-  return FALSE;
-}
-
-
 String *Item::check_well_formed_result(String *str, bool send_error)
 {
   /* Check whether we got a well-formed string */
@@ -7468,6 +7449,8 @@ bool  Item_cache_datetime::cache_value_int()
     return FALSE;
 
   value_cached= TRUE;
+  // Mark cached string value obsolete
+  str_value_cached= FALSE;
   /* Assume here that the underlying item will do correct conversion.*/
   int_value= example->val_int_result();
   null_value= example->null_value;
@@ -7480,7 +7463,13 @@ bool  Item_cache_datetime::cache_value()
 {
   if (!example)
     return FALSE;
+
+  if (cmp_context == INT_RESULT)
+    return cache_value_int();
+
   str_value_cached= TRUE;
+  // Mark cached int value obsolete
+  value_cached= FALSE;
   /* Assume here that the underlying item will do correct conversion.*/
   String *res= example->str_result(&str_value);
   if (res && res != &str_value)
@@ -7504,8 +7493,47 @@ void Item_cache_datetime::store(Item *item, longlong val_arg)
 String *Item_cache_datetime::val_str(String *str)
 {
   DBUG_ASSERT(fixed == 1);
-  if (!str_value_cached && !cache_value())
+  if (!str_value_cached)
+  {
+    /*
+      When it's possible the Item_cache_datetime uses INT datetime
+      representation due to speed reasons. But still, it always has the STRING
+      result type and thus it can be asked to return a string value. 
+      It is possible that at this time cached item doesn't contain correct
+      string value, thus we have to convert cached int value to string and
+      return it.
+    */
+    if (value_cached)
+    {
+      MYSQL_TIME ltime;
+      if (str_value.alloc(MAX_DATE_STRING_REP_LENGTH))
+        return NULL;
+      if (cached_field_type == MYSQL_TYPE_TIME)
+      {
+        ulonglong time= int_value;
+        DBUG_ASSERT(time < TIME_MAX_VALUE);
+        set_zero_time(&ltime, MYSQL_TIMESTAMP_TIME);
+        ltime.second= time % 100;
+        time/= 100;
+        ltime.minute= time % 100;
+        time/= 100;
+        ltime.hour= time % 100;
+      }
+      else
+      {
+        int was_cut;
+        longlong res;
+        res= number_to_datetime(val_int(), &ltime, TIME_FUZZY_DATE, &was_cut);
+        if (res == -1)
+          return NULL;
+      }
+      str_value.length(my_TIME_to_str(&ltime,
+                                      const_cast<char*>(str_value.ptr())));
+      str_value_cached= TRUE;
+    }
+    else if (!cache_value())
       return NULL;
+  }
   return &str_value;
 }
 

sql/item.h

@@ -1171,7 +1171,40 @@ public:
     representation is more precise than the string one).
   */
   virtual bool result_as_longlong() { return FALSE; }
-  bool is_datetime();
+  inline bool is_datetime() const
+  {
+    switch (field_type())
+    {
+      case MYSQL_TYPE_DATE:
+      case MYSQL_TYPE_DATETIME:
+      case MYSQL_TYPE_TIMESTAMP:
+        return TRUE;
+      default:
+        break;
+    }
+    return FALSE;
+  }
+  /**
+    Check whether this and the given item has compatible comparison context.
+    Used by the equality propagation. See Item_field::equal_fields_propagator.
+
+    @return
+      TRUE  if the context is the same or if fields could be
+            compared as DATETIME values by the Arg_comparator.
+      FALSE otherwise.
+  */
+  inline bool has_compatible_context(Item *item) const
+  {
+    /* Same context. */
+    if (cmp_context == (Item_result)-1 || item->cmp_context == cmp_context)
+      return TRUE;
+    /* DATETIME comparison context. */
+    if (is_datetime())
+      return item->is_datetime() || item->cmp_context == STRING_RESULT;
+    if (item->is_datetime())
+      return is_datetime() || cmp_context == STRING_RESULT;
+    return FALSE;
+  }
   virtual Field::geometry_type get_geometry_type() const
     { return Field::GEOM_GEOMETRY; };
   String *check_well_formed_result(String *str, bool send_error= 0);
@@ -3232,6 +3265,7 @@ public:
   virtual bool cache_value()= 0;
   bool basic_const_item() const
   { return test(example && example->basic_const_item());}
+  virtual void clear() { null_value= TRUE; value_cached= FALSE; }
 };
 
 
@@ -3412,6 +3446,7 @@ public:
   */
   bool cache_value_int();
   bool cache_value();
+  void clear() { Item_cache::clear(); str_value_cached= FALSE; }
 };
 
 

sql/item_cmpfunc.cc

@@ -876,7 +876,9 @@ get_time_value(THD *thd, Item ***item_arg, Item **cache_arg,
     Do not cache GET_USER_VAR() function as its const_item() may return TRUE
     for the current thread but it still may change during the execution.
   */
-  if (item->const_item() && cache_arg && (item->type() != Item::FUNC_ITEM ||
+  if (item->const_item() && cache_arg &&
+      item->type() != Item::CACHE_ITEM &&
+      (item->type() != Item::FUNC_ITEM ||
        ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC))
   {
     Item_cache_int *cache= new Item_cache_int();
@@ -937,6 +939,7 @@ int Arg_comparator::set_cmp_func(Item_result_field *owner_arg,
     get_value_a_func= &get_datetime_value;
     get_value_b_func= &get_datetime_value;
     cmp_collation.set(&my_charset_numeric);
+    set_cmp_context_for_datetime();
     return 0;
   }
   else if (type == STRING_RESULT && (*a)->field_type() == MYSQL_TYPE_TIME &&
@@ -949,6 +952,7 @@ int Arg_comparator::set_cmp_func(Item_result_field *owner_arg,
     func= &Arg_comparator::compare_datetime;
     get_value_a_func= &get_time_value;
     get_value_b_func= &get_time_value;
+    set_cmp_context_for_datetime();
     return 0;
   }
   else if (type == STRING_RESULT &&
@@ -1005,6 +1009,7 @@ bool Arg_comparator::try_year_cmp_func(Item_result type)
 
   is_nulls_eq= is_owner_equal_func();
   func= &Arg_comparator::compare_datetime;
+  set_cmp_context_for_datetime();
 
   return TRUE;
 }
@@ -1058,6 +1063,7 @@ void Arg_comparator::set_datetime_cmp_func(Item_result_field *owner_arg,
   func= &Arg_comparator::compare_datetime;
   get_value_a_func= &get_datetime_value;
   get_value_b_func= &get_datetime_value;
+  set_cmp_context_for_datetime();
 }
 
 
@@ -1144,7 +1150,9 @@ get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg,
     Do not cache GET_USER_VAR() function as its const_item() may return TRUE
     for the current thread but it still may change during the execution.
   */
-  if (item->const_item() && cache_arg && (item->type() != Item::FUNC_ITEM ||
+  if (item->const_item() && cache_arg &&
+      item->type() != Item::CACHE_ITEM &&
+      (item->type() != Item::FUNC_ITEM ||
        ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC))
   {
     Item_cache_int *cache= new Item_cache_int(MYSQL_TYPE_DATETIME);

sql/item_cmpfunc.h

@@ -123,7 +123,17 @@ public:
     delete [] comparators;
     comparators= 0;
   }
-
+  /*
+    Set correct cmp_context if items would be compared as INTs.
+  */
+  inline void set_cmp_context_for_datetime()
+  {
+    DBUG_ASSERT(func == &Arg_comparator::compare_datetime);
+    if ((*a)->result_as_longlong())
+      (*a)->cmp_context= INT_RESULT;
+    if ((*b)->result_as_longlong())
+      (*b)->cmp_context= INT_RESULT;
+  }
   friend class Item_func;
 };
 

sql/item_sum.cc

@@ -1214,7 +1214,6 @@ void Item_sum_hybrid::setup_hybrid(Item *item, Item *value_arg)
 {
   value= Item_cache::get_cache(item);
   value->setup(item);
-  if (value_arg)
   value->store(value_arg);
   cmp= new Arg_comparator();
   cmp->set_cmp_func(this, args, (Item**)&value, FALSE);
@@ -1903,7 +1902,7 @@ void Item_sum_variance::update_field()
 
 void Item_sum_hybrid::clear()
 {
-  value->null_value= 1;
+  value->clear();
   null_value= 1;
 }
 

sql/item_sum.h

@@ -1010,6 +1010,11 @@ protected:
   void no_rows_in_result();
   Field *create_tmp_field(bool group, TABLE *table,
 			  uint convert_blob_length);
+  /*
+    MIN/MAX uses Item_cache_datetime for storing DATETIME values, thus
+    in this case a correct INT value can be provided.
+  */
+  bool result_as_longlong() { return args[0]->result_as_longlong(); }
 };