List:Commits« Previous MessageNext Message »
From:Evgeny Potemkin Date:October 7 2010 6:36am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3210) Bug#57039
View as plain text  
hi Oystein,

Thanks for the review:)

On 10/06/10 14:04, Øystein Grøvlen wrote:
> Evgeny,
>
> Approved. See some minor suggestions inline.
>
> On 10/01/2010 09:12 AM, Evgeny Potemkin wrote:
>> #At file:///work/bzrroot/57039-bug-5.5-bugfixing/ based on
>> revid:georgi.kodinov@stripped
>>
>> 3210 Evgeny Potemkin 2010-10-01
>> Bug#57039: constant subtime expression returns incorrect result.
>> The subtime function wasn't able to produce correct int representation of
>> its result. For constant expressions the Item_datetime_cache is used to
>> speedup evaluation and Item_datetime_cache expects underlying item to return
>> correct int representation of DATETIME value. These two factors combined led
>> to a wrong query result.
>>
>> Now the Item_func_add_time has function val_datetime which performs the
>> calculation and saves result into given MYSQL_TIME struct, it also sets
>> null_value to appropriate value. val_int and val_str member functions
>> convert the result obtained from val_datetime to int or string respectively
>> and returns it.
>> @ mysql-test/r/func_time.result
>> Added a test case for the bug#57039.
>> @ mysql-test/t/func_time.test
>> Added a test case for the bug#57039.
>> @ sql/item_timefunc.cc
>> Bug#57039: constant subtime expression returns incorrect result.
>> Now the Item_func_add_time has function val_datetime which performs the
>> calculation and saves result into given MYSQL_TIME struct, it also sets
>> null_value to appropriate value. val_int and val_str member functions
>> convert the result obtained from val_datetime to int or string respectively
>> and returns it.
>> @ sql/item_timefunc.h
>> Bug#57039: constant subtime expression returns incorrect result.
>>
>> modified:
>> mysql-test/r/func_time.result
>> mysql-test/t/func_time.test
>> sql/item_timefunc.cc
>> sql/item_timefunc.h
>> === modified file 'mysql-test/r/func_time.result'
>> --- a/mysql-test/r/func_time.result 2010-09-09 12:02:02 +0000
>> +++ b/mysql-test/r/func_time.result 2010-10-01 07:12:55 +0000
>> @@ -1316,3 +1316,12 @@ SELECT 1 FROM t1 ORDER BY @x:=makedate(a
>> 1
>> DROP TABLE t1;
>> End of 5.1 tests
>> +#
>> +# Bug#57039: constant subtime expression returns incorrect result.
>> +#
>> +CREATE TABLE t1 (`date_date` datetime NOT NULL);
>> +INSERT INTO t1 VALUES ('2008-01-03 00:00:00'), ('2008-01-03 00:00:00');
>> +SELECT * FROM t1 WHERE date_date>= subtime(now(), "00:30:00");
>> +date_date
>> +DROP TABLE t1;
>> +#
>>
>> === modified file 'mysql-test/t/func_time.test'
>> --- a/mysql-test/t/func_time.test 2010-08-16 07:11:57 +0000
>> +++ b/mysql-test/t/func_time.test 2010-10-01 07:12:55 +0000
>> @@ -833,3 +833,13 @@ SELECT 1 FROM t1 ORDER BY @x:=makedate(a
>> DROP TABLE t1;
>>
>> --echo End of 5.1 tests
>> +
>> +--echo #
>> +--echo # Bug#57039: constant subtime expression returns incorrect result.
>> +--echo #
>> +CREATE TABLE t1 (`date_date` datetime NOT NULL);
>> +INSERT INTO t1 VALUES ('2008-01-03 00:00:00'), ('2008-01-03 00:00:00');
>> +SELECT * FROM t1 WHERE date_date>= subtime(now(), "00:30:00");
>> +DROP TABLE t1;
>> +--echo #
>> +
>>
>
> Would it be an idea to have a test case with addtime, too? And maybe something
> that uses less than instead of greater thaN?
It's effectively the same function, just with different name. But if you think 
it's worth adding I'll do.
>
>> === modified file 'sql/item_timefunc.cc'
>> --- a/sql/item_timefunc.cc 2010-09-07 06:45:00 +0000
>> +++ b/sql/item_timefunc.cc 2010-10-01 07:12:55 +0000
>> @@ -52,6 +52,7 @@
>> #include "sql_class.h" // THD
>> #include<m_ctype.h>
>> #include<time.h>
>> +#include "item_cmpfunc.h" // get_date_from_str
>
> I have not tested if this inclusion is needed, but I do not see any use of
> get_date_from_str, so the comment should at least go away.
Sorry, it's a leftover from a first try.
>
>>
>> /** Day number for Dec 31st, 9999. */
>> #define MAX_DAY_NUMBER 3652424L
>> @@ -2857,10 +2858,11 @@ void Item_func_add_time::fix_length_and_
>> Result: Time value or datetime value
>> */
>>
>> -String *Item_func_add_time::val_str(String *str)
>> +MYSQL_TIME *Item_func_add_time::val_datetime(MYSQL_TIME *time,
>> + date_time_format_types *format)
>> {
>> DBUG_ASSERT(fixed == 1);
>> - MYSQL_TIME l_time1, l_time2, l_time3;
>> + MYSQL_TIME l_time1, l_time2;
>> bool is_time= 0;
>> long days, microseconds;
>> longlong seconds;
>> @@ -2886,41 +2888,38 @@ String *Item_func_add_time::val_str(Stri
>> if (l_time1.neg != l_time2.neg)
>> l_sign= -l_sign;
>>
>> - bzero((char *)&l_time3, sizeof(l_time3));
>> + bzero((char *)time, sizeof(MYSQL_TIME));
>>
>> - l_time3.neg= calc_time_diff(&l_time1,&l_time2, -l_sign,
>> - &seconds,&microseconds);
>> + time->neg= calc_time_diff(&l_time1,&l_time2, -l_sign,
>> +&seconds,&microseconds);
>>
>> /*
>> If first argument was negative and diff between arguments
>> is non-zero we need to swap sign to get proper result.
>> */
>> if (l_time1.neg&& (seconds || microseconds))
>> - l_time3.neg= 1-l_time3.neg; // Swap sign of result
>> + time->neg= 1 - time->neg; // Swap sign of result
>>
>> - if (!is_time&& l_time3.neg)
>> + if (!is_time&& time->neg)
>> goto null_date;
>>
>> days= (long)(seconds/86400L);
>>
>> - calc_time_from_sec(&l_time3, (long)(seconds%86400L), microseconds);
>> + calc_time_from_sec(time, (long)(seconds%86400L), microseconds);
>>
>> if (!is_time)
>> {
>> -
> get_date_from_daynr(days,&l_time3.year,&l_time3.month,&l_time3.day);
>> - if (l_time3.day&&
>> - !make_datetime(l_time1.second_part || l_time2.second_part ?
>> - DATE_TIME_MICROSECOND : DATE_TIME,
>> - &l_time3, str))
>> - return str;
>> +
> get_date_from_daynr(days,&time->year,&time->month,&time->day);
>> + *format= l_time1.second_part || l_time2.second_part ?
>> + DATE_TIME_MICROSECOND : DATE_TIME;
>> + if (time->day)
>> + return time;
>> goto null_date;
>> }
>> -
>> - l_time3.hour+= days*24;
>> - if (!make_datetime_with_warn(l_time1.second_part || l_time2.second_part ?
>> - TIME_MICROSECOND : TIME_ONLY,
>> -&l_time3, str))
>> - return str;
>> + *format= l_time1.second_part || l_time2.second_part ?
>> + TIME_MICROSECOND : TIME_ONLY;
>> + time->hour+= days*24;
>> + return time;
>>
>> null_date:
>> null_value=1;
>> @@ -2928,6 +2927,38 @@ null_date:
>> }
>>
>>
>> +String *Item_func_add_time::val_str(String *str)
>> +{
>> + MYSQL_TIME ltime;
>> + date_time_format_types format;
>> +
>> + val_datetime(&ltime,&format);
>> +
>> + if (null_value)
>> + return 0;
>> +
>> + if (!make_datetime_with_warn(format,&ltime, str))
>> + return str;
>> +
>> + null_value= 1;
>> + return 0;
>> +}
>> +
>> +
>> +longlong Item_func_add_time::val_int()
>> +{
>> + MYSQL_TIME ltime;
>> + date_time_format_types format;
>> +
>> + val_datetime(&ltime,&format);
>> +
>> + if (null_value)
>> + return 0;
>> +
>> + return TIME_to_ulonglong_datetime(&ltime);
>> +}
>> +
>> +
>> void Item_func_add_time::print(String *str, enum_query_type query_type)
>> {
>> if (is_date)
>>
>> === modified file 'sql/item_timefunc.h'
>> --- a/sql/item_timefunc.h 2010-09-09 12:02:02 +0000
>> +++ b/sql/item_timefunc.h 2010-10-01 07:12:55 +0000
>> @@ -949,6 +949,8 @@ public:
>> return save_date_in_field(field);
>> return Item_str_func::save_in_field(field, no_conversions);
>> }
>> + longlong val_int();
>> + MYSQL_TIME *val_datetime(MYSQL_TIME *time, date_time_format_types *format);
>> };
>>
>> class Item_func_timediff :public Item_str_timefunc
>>
>>
>>
>>
>>
>
> --
> Øystein
>
>
Regards, Evgen.
Thread
bzr commit into mysql-5.5-bugteam branch (epotemkin:3210) Bug#57039Evgeny Potemkin1 Oct
  • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3210) Bug#57039Øystein Grøvlen6 Oct
    • Re: bzr commit into mysql-5.5-bugteam branch (epotemkin:3210) Bug#57039Evgeny Potemkin7 Oct