List:Commits« Previous MessageNext Message »
From:Alexey Kopytov Date:June 9 2010 5:27pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)
Bug#49771
View as plain text  
Hi Evgeny,

My apologies for a very delayed review. Some comments and questions:

On 25.05.10 16:07, Evgeny Potemkin wrote:
> #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.

[skip]

>
> === 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;
>       }

So in both places where is_datetime_comparable() is invoked we also 
require "cmp_context != (Item_result)-1 && item->cmp_context != 
cmp_context". Does it make sense to move this logic to 
is_datetime_comparable()? It would then be logical to rename it to 
something like has_compatible_context() and move the entire comment 
above the first "if" there.

> @@ -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 {

Braces above should be on separate lines.

> +        /*
> +          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))

Does it make sense to call number_to_datetime() directly instead of 
get_date()? This way you don't need to play tricks with 
cached_result_type. You also don't get validation and error handling 
performed in Item::get_date, but is that really needed? For the 
"cached_field_type == MYSQL_TYPE_TIME" you seem to assume the input to 
be always correct.

> +        {
> +          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()));

Why not just "str_value.length(my_TIME_to_str(&ltime, 
(char*)str_value.ptr()));"?

> +      str_value_cached= TRUE;
> +    }
> +    else if (!cache_value())
> +      return NULL;

Could it be that both str_value_cached and value_cached are FALSE, so we 
have to invoke cache_value(), but the cached item is not in a correct 
state anymore and thus, we are back to the original problem?

> +  }
>     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()
> +  {

The "inline" keyword above is redundant because a member function 
defined in the class body is inline by definition. I refreshed my memory 
on this just recently, some of my last patches contain the redundant 
"inline" too :).

> +    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;

I vote on simplifying the above if (perhaps together with other proposed 
changes to this function, if you will agree on them). IIUC, its 
expression is true if and only if for at least one of its "arguments" 
is_datetime() is true and the other one has either is_datetime() being 
true as well or has the STRING_RESULT context.

Would the following be a more readable check?

     if (is_datetime())
       return item->is_datetime() || item->cmp_context == STRING_RESULT;
     else 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);
> @@ -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)

Could you clarify this change?

>     {
>       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;

Shouldn't the above be an assertion?

> +    if ((*a)->result_as_longlong())
> +      (*a)->cmp_context= INT_RESULT;
> +    if ((*b)->result_as_longlong())
> +      (*b)->cmp_context= INT_RESULT;
> +  }
>     friend class Item_func;
>   };
>
>

[skip]

Best regards,
Alexey.
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