List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:May 25 2010 12:07pm
Subject:bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)
Bug#49771
View as plain text  
#At file:///work/bzrroot/49771-bug-next-mr-bugfixing/ based on revid:mattias.jonsson@stripped

 3200 Evgeny Potemkin	2010-05-25
      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::is_datetime_comparable helper function is added. It checks whether
      this and given items 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 get_date & 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.
     @ mysql-test/t/group_by.test
        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::is_datetime_comparable helper function is added. It checks whether
        this and given items 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.

    modified:
      mysql-test/r/group_by.result
      mysql-test/t/group_by.test
      sql/item.cc
      sql/item.h
      sql/item_cmpfunc.cc
      sql/item_cmpfunc.h
      sql/item_sum.cc
      sql/item_sum.h
=== modified file 'mysql-test/r/group_by.result'
--- a/mysql-test/r/group_by.result	2010-03-24 15:03:44 +0000
+++ b/mysql-test/r/group_by.result	2010-05-25 12:07:47 +0000
@@ -1792,3 +1792,41 @@ aa	b	COUNT(         b)
 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');
+INSERT INTO t1 VALUES (1,'0000-00-00');
+INSERT INTO t1 VALUES (1,'2004-04-18');
+INSERT INTO t1 VALUES (2,'2004-05-19');
+INSERT INTO t1 VALUES (2,'0001-01-01');
+INSERT INTO t1 VALUES (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');
+INSERT INTO t1 VALUES (1,'06:11:01');
+INSERT INTO t1 VALUES (2,'19:53:05');
+INSERT INTO t1 VALUES (2,'21:44:25');
+INSERT INTO t1 VALUES (3,'10:55:12');
+INSERT INTO t1 VALUES (3,'05:45:11');
+INSERT INTO t1 VALUES (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

=== modified file 'mysql-test/t/group_by.test'
--- a/mysql-test/t/group_by.test	2010-02-24 13:52:27 +0000
+++ b/mysql-test/t/group_by.test	2010-05-25 12:07:47 +0000
@@ -1209,3 +1209,35 @@ 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');
+INSERT INTO t1 VALUES (1,'0000-00-00');
+INSERT INTO t1 VALUES (1,'2004-04-18');
+INSERT INTO t1 VALUES (2,'2004-05-19');
+INSERT INTO t1 VALUES (2,'0001-01-01');
+INSERT INTO t1 VALUES (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');
+INSERT INTO t1 VALUES (1,'06:11:01');
+INSERT INTO t1 VALUES (2,'19:53:05');
+INSERT INTO t1 VALUES (2,'21:44:25');
+INSERT INTO t1 VALUES (3,'10:55:12');
+INSERT INTO t1 VALUES (3,'05:45:11');
+INSERT INTO t1 VALUES (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

=== modified file 'sql/item.cc'
--- a/sql/item.cc	2010-05-05 13:57:29 +0000
+++ b/sql/item.cc	2010-05-25 12:07:47 +0000
@@ -4906,11 +4906,12 @@ Item *Item_field::equal_fields_propagato
     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.
+    DATE/DATETIME fields are exception and because of this they are checked
+    by the is_datetime_comparable function.
   */
   if (!item ||
-      (cmp_context != (Item_result)-1 && item->cmp_context != cmp_context))
+      (cmp_context != (Item_result)-1 && item->cmp_context != cmp_context &&
+       !is_datetime_comparable(item)))
     item= this;
   else if (field && (field->flags & ZEROFILL_FLAG) && IS_NUM(field->type()))
   {
@@ -4975,7 +4976,8 @@ Item *Item_field::replace_equal_field(uc
     if (const_item)
     {
       if (cmp_context != (Item_result)-1 &&
-          const_item->cmp_context != cmp_context)
+          const_item->cmp_context != cmp_context &&
+          !is_datetime_comparable(const_item))
         return this;
       return const_item;
     }
@@ -5045,21 +5047,6 @@ enum_field_types Item::field_type() cons
 }
 
 
-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 */
@@ -7440,6 +7427,8 @@ bool  Item_cache_datetime::cache_value_i
     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;
@@ -7452,7 +7441,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)
@@ -7476,8 +7471,52 @@ void Item_cache_datetime::store(Item *it
 String *Item_cache_datetime::val_str(String *str)
 {
   DBUG_ASSERT(fixed == 1);
-  if (!str_value_cached && !cache_value())
-    return NULL;
+  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;
+        memset(&ltime, 0, sizeof(MYSQL_TIME));
+        ltime.second= time % 100;
+        time/= 100;
+        ltime.minute= time % 100;
+        time/= 100;
+        ltime.hour= time % 100;
+        DBUG_ASSERT(!(time/100));
+        ltime.time_type= MYSQL_TIMESTAMP_TIME;
+      } else {
+        /*
+          get_date uses result type to determine the type of the source
+          value. Tweak it to get datetime from cached int value.
+        */
+        cached_result_type= INT_RESULT;
+        if (get_date(&ltime, TIME_FUZZY_DATE))
+        {
+          cached_result_type= STRING_RESULT;
+          return NULL;
+        }
+        cached_result_type= STRING_RESULT;
+      }
+      my_TIME_to_str(&ltime, (char*)str_value.ptr());
+      str_value.length(strlen(str_value.ptr()));
+      str_value_cached= TRUE;
+    }
+    else if (!cache_value())
+      return NULL;
+  }
   return &str_value;
 }
 

=== modified file 'sql/item.h'
--- a/sql/item.h	2010-04-13 22:07:08 +0000
+++ b/sql/item.h	2010-05-25 12:07:47 +0000
@@ -1162,7 +1162,33 @@ public:
     representation is more precise than the string one).
   */
   virtual bool result_as_longlong() { return FALSE; }
-  bool is_datetime();
+  bool inline is_datetime()
+  {
+    switch (field_type())
+    {
+      case MYSQL_TYPE_DATE:
+      case MYSQL_TYPE_DATETIME:
+      case MYSQL_TYPE_TIMESTAMP:
+        return TRUE;
+      default:
+        break;
+    }
+    return FALSE;
+  }
+  /*
+    Fast check whether given item can be compared with this item as datetime
+    by Arg_comparator.
+  */
+  bool inline is_datetime_comparable(Item *item)
+  {
+    bool datetime= FALSE;
+    if (((datetime= is_datetime()) || cmp_context == STRING_RESULT) &&
+        ((datetime|= item->is_datetime()) ||
+          item->cmp_context == STRING_RESULT) &&
+          datetime)
+      return TRUE;
+    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);
@@ -3193,6 +3219,7 @@ public:
   virtual bool cache_value()= 0;
   bool basic_const_item() const
   { return test(example && example->basic_const_item());}
+  virtual void clear() { null_value= 1; value_cached= FALSE; }
 };
 
 
@@ -3349,11 +3376,12 @@ protected:
   String str_value;
   ulonglong int_value;
   bool str_value_cached;
+  Item_result cached_result_type;
 public:
   Item_cache_datetime(enum_field_types field_type_arg):
     Item_cache(field_type_arg), int_value(0), str_value_cached(0)
   {
-    cmp_context= STRING_RESULT;
+    cmp_context= cached_result_type= STRING_RESULT;
   }
 
   void store(Item *item, longlong val_arg);
@@ -3361,7 +3389,7 @@ public:
   longlong val_int();
   String* val_str(String *str);
   my_decimal *val_decimal(my_decimal *);
-  enum Item_result result_type() const { return STRING_RESULT; }
+  enum Item_result result_type() const { return cached_result_type; }
   bool result_as_longlong() { return TRUE; }
   /*
     In order to avoid INT <-> STRING conversion of a DATETIME value
@@ -3372,6 +3400,7 @@ public:
   */
   bool cache_value_int();
   bool cache_value();
+  void clear() { null_value= 1; value_cached= str_value_cached= FALSE; }
 };
 
 

=== modified file 'sql/item_cmpfunc.cc'
--- a/sql/item_cmpfunc.cc	2010-03-31 14:05:33 +0000
+++ b/sql/item_cmpfunc.cc	2010-05-25 12:07:47 +0000
@@ -877,7 +877,8 @@ get_time_value(THD *thd, Item ***item_ar
     for the current thread but it still may change during the execution.
   */
   if (item->const_item() && cache_arg && (item->type() != Item::FUNC_ITEM ||
-      ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC))
+      ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC) &&
+      item->type() != Item::CACHE_ITEM)
   {
     Item_cache_int *cache= new Item_cache_int();
     /* Mark the cache as non-const to prevent re-caching. */
@@ -937,6 +938,7 @@ int Arg_comparator::set_cmp_func(Item_re
     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 +951,7 @@ int Arg_comparator::set_cmp_func(Item_re
     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 +1008,7 @@ bool Arg_comparator::try_year_cmp_func(I
 
   is_nulls_eq= is_owner_equal_func();
   func= &Arg_comparator::compare_datetime;
+  set_cmp_context_for_datetime();
 
   return TRUE;
 }
@@ -1058,6 +1062,7 @@ void Arg_comparator::set_datetime_cmp_fu
   func= &Arg_comparator::compare_datetime;
   get_value_a_func= &get_datetime_value;
   get_value_b_func= &get_datetime_value;
+  set_cmp_context_for_datetime();
 }
 
 
@@ -1145,7 +1150,8 @@ get_datetime_value(THD *thd, Item ***ite
     for the current thread but it still may change during the execution.
   */
   if (item->const_item() && cache_arg && (item->type() != Item::FUNC_ITEM ||
-      ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC))
+      ((Item_func*)item)->functype() != Item_func::GUSERVAR_FUNC) &&
+      item->type() != Item::CACHE_ITEM)
   {
     Item_cache_int *cache= new Item_cache_int(MYSQL_TYPE_DATETIME);
     /* Mark the cache as non-const to prevent re-caching. */

=== modified file 'sql/item_cmpfunc.h'
--- a/sql/item_cmpfunc.h	2010-04-01 19:34:09 +0000
+++ b/sql/item_cmpfunc.h	2010-05-25 12:07:47 +0000
@@ -118,7 +118,18 @@ public:
     return (owner->type() == Item::FUNC_ITEM &&
            ((Item_func*)owner)->functype() == Item_func::EQUAL_FUNC);
   }
-
+  /*
+    Set correct cmp_context if items will be compared as INTs.
+  */
+  void inline set_cmp_context_for_datetime()
+  {
+    if (func != &Arg_comparator::compare_datetime)
+      return;
+    if ((*a)->result_as_longlong())
+      (*a)->cmp_context= INT_RESULT;
+    if ((*b)->result_as_longlong())
+      (*b)->cmp_context= INT_RESULT;
+  }
   friend class Item_func;
 };
 

=== modified file 'sql/item_sum.cc'
--- a/sql/item_sum.cc	2010-04-09 11:20:40 +0000
+++ b/sql/item_sum.cc	2010-05-25 12:07:47 +0000
@@ -1217,8 +1217,7 @@ void Item_sum_hybrid::setup_hybrid(Item 
 {
   value= Item_cache::get_cache(item);
   value->setup(item);
-  if (value_arg)
-    value->store(value_arg);
+  value->store(value_arg);
   cmp= new Arg_comparator();
   cmp->set_cmp_func(this, args, (Item**)&value, FALSE);
   collation.set(item->collation);
@@ -1906,7 +1905,7 @@ void Item_sum_variance::update_field()
 
 void Item_sum_hybrid::clear()
 {
-  value->null_value= 1;
+  value->clear();
   null_value= 1;
 }
 

=== modified file 'sql/item_sum.h'
--- a/sql/item_sum.h	2010-04-09 11:20:40 +0000
+++ b/sql/item_sum.h	2010-05-25 12:07:47 +0000
@@ -1006,6 +1006,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 inline result_as_longlong() { return args[0]->result_as_longlong(); }
 };
 
 


Attachment: [text/bzr-bundle] bzr/epotemkin@mysql.com-20100525120747-hg7pfk7b90gz43vc.bundle
Thread
bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Evgeny Potemkin25 May
  • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Martin Hansson1 Jun
  • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Alexey Kopytov9 Jun
    • Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)Bug#49771Evgeny Potemkin12 Jul