List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:July 12 2010 9:09am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (epotemkin:3200)
Bug#49771
View as plain text  
Hi Alexey,

On 06/09/10 21:27, Alexey Kopytov wrote:
> 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.
Done.
>
>> @@ -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.
Done.
>
>> + /*
>> + 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.
Done.
>
>> + {
>> + 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?
Yes, it's possible. I suppose the 'correct state' means non-NULL.
If underlying item isn't set (NULL) then on each call we'll return NULL, also
xxx_cached values won't be set, so on each call we'll call cache_value() and try 
to cache it.

>
>> + }
>> 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 :).
Ok. I left it to see that this function is inline without looking at the parent 
class.
>
>> + 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;
Done.
>
>> + }
>> 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?
The idea is to not re-cache already cached items since it's useless.
>
>> {
>> 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?
Done.
>
>> + 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.
>
Regards, Evgen.
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