MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:November 17 2009 1:56pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3165) Bug#43668
View as plain text  
Hi Sergey,

Sergei Golubchik wrote:
> Hi, Evgeny!
> 
> On Nov 15, Evgeny Potemkin wrote:
>>  3165 Evgeny Potemkin	2009-11-15
>>       Bug#43668: Wrong comparison and MIN/MAX for YEAR(2)
> 
> Just one comment - I thought that you will remove tmp_value1/tmp_value2
> from Item_bool_func2 - as you have them in the Arg_comparator now.
> Any reason for not doing it ?
No, I just forgot to do it. Sorry.
>        
>> === modified file 'sql/item_cmpfunc.cc'
>> --- a/sql/item_cmpfunc.cc	2009-10-05 05:27:36 +0000
>> +++ b/sql/item_cmpfunc.cc	2009-11-15 16:55:51 +0000
>> @@ -1031,6 +1057,51 @@ get_datetime_value(THD *thd, Item ***ite
>>    return value;
>>  }
>>  
>> +
>> +/*
>> +  Retrieves YEAR value of 19XX form from given item.
>> +
>> +  SYNOPSIS
>> +    get_year_value()
>> +    thd                 thread handle
>> +    item_arg   [in/out] item to retrieve YEAR value from
>> +    cache_arg  [in/out] pointer to place to store the caching item to
>> +    warn_item  [in]     item for issuing the conversion warning
>> +    is_null    [out]    TRUE <=> the item_arg is null
>> +
>> +  DESCRIPTION
>> +    Retrieves the YEAR value of 19XX form from given item for comparison by the
>> +    compare_year() function.
>> +
>> +  RETURN
>> +    obtained value
>> +*/
>> +
>> +static longlong
> 
> heh, longlong makes it for an awful lot of years :)
> ok, whatever...
It's because of the get_value_func prototype.
> 
>> +get_year_value(THD *thd, Item ***item_arg, Item **cache_arg,
>> +               Item *warn_item, bool *is_null)
>> +{
>> +  longlong value= 0;
>> +  Item *item= **item_arg;
>> +
>> +  value= item->val_int();
>> +  *is_null= item->null_value;
>> +  if (*is_null)
>> +    return ~(ulonglong) 0;
>> +
>> +  /*
>> +    Coerce value to the 19XX form in order to correctly compare
>> +    YEAR(2) & YEAR(4) types.
>> +  */
>> +  if (value < 70)
>> +    value+= 100;
>> +  if (value <= 1900)
>> +    value+= 1900;
>> +
>> +  return value;
>> +}
>> +
>> +
>>  /*
>>    Compare items values as dates.
>>  
>> @@ -1437,6 +1527,67 @@ int Arg_comparator::compare_e_row()
>>  }
>>  
>>  
>> +/**
>> +  Compare values as YEAR.
>> +
>> +  @details
>> +    Compare items as YEAR for EQUAL_FUNC and for other comparison functions.
>> +    The YEAR values of form 19XX are obtained with help of the get_year_value()
>> +    function.
>> +    If one of arguments is of DATE/DATETIME type its value is obtained
>> +    with help of the get_datetime_value function. In this case YEAR values
>> +    prior to comparison are converted to the ulonglong YYYY-00-00 00:00:00
>> +    DATETIME form.
>> +    If an argument type neither YEAR nor DATE/DATEIME then val_int function
>> +    is used to obtain value for comparison.
>> +
>> +  RETURN
>> +    If is_nulls_eq is TRUE:
>> +       1    if items are equal or both are null
>> +       0    otherwise
>> +    If is_nulls_eq is FALSE:
>> +      -1   a < b
>> +       0   a == b or at least one of items is null
>> +       1   a > b
>> +    See the table:
>> +    is_nulls_eq | 1 | 1 | 1 | 1 | 0 | 0 | 0 | 0 |
>> +    a_is_null   | 1 | 0 | 1 | 0 | 1 | 0 | 1 | 0 |
>> +    b_is_null   | 1 | 1 | 0 | 0 | 1 | 1 | 0 | 0 |
>> +    result      | 1 | 0 | 0 |0/1| 0 | 0 | 0 |-1/0/1|
> 
> I personally don't like the idea of is_nulls_eq and merging two
> functions into one. The return values are very different:
> 
>    if is_nulls_eq     - it returns 1 for equal and 0 for not equal
>    if not is_nulls_eq - it returns -1/0/1 for less/equal/greater
> 
> so in my opinion it'd be clearer to keep these functions separate.
> 
> But as somebody has already introduced this "is_nulls_eq" concept, you
> can use it of course, I'm not asking to change that.
It was introduced by me in the fix for DATE/DATEIME comparison.
IMHO it's better to have only one function to fix/extend when needed, but I can 
split it.
> 
>> +*/
>> +
>> +int Arg_comparator::compare_year()
>> +{
>> +  bool a_is_null, b_is_null;
>> +  ulonglong val1= get_value_a_func ?
>> +                    (*get_value_a_func)(thd, &a, &a_cache, *b,
> &a_is_null) :
>> +                    (*a)->val_int();
>> +  ulonglong val2= get_value_b_func ?
>> +                    (*get_value_b_func)(thd, &b, &b_cache, *a,
> &b_is_null) :
>> +                    (*b)->val_int();
>> +  if (!(*a)->null_value)
>> +  {
>> +    if (!(*b)->null_value)
>> +    {
>> +      if (set_null)
>> +        owner->null_value= 0;
>> +      /* Convert year to DATETIME of form YYYY-00-00 00:00:00 when necessary.
> */
>> +      if((*a)->field_type() == MYSQL_TYPE_YEAR && year_as_datetime)
>> +        val1*= 10000000000LL;
>> +      if((*b)->field_type() == MYSQL_TYPE_YEAR && year_as_datetime)
>> +        val2*= 10000000000LL;
> 
> and you have tests for this too, good.
> 
> Regards / Mit vielen Grüßen,
> Sergei
> 
Thread
bzr commit into mysql-5.1-bugteam branch (epotemkin:3165) Bug#43668Evgeny Potemkin15 Nov
  • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3165)Bug#43668Sergei Golubchik17 Nov
    • Re: bzr commit into mysql-5.1-bugteam branch (epotemkin:3165) Bug#43668Evgeny Potemkin17 Nov