Hi Sergei, Thanks for review! On 06/11/2013 09:33 PM, Sergei Golubchik wrote:
Hi, Alexander!
This looks like a lot of changes for what you explained the reason of the bug was. There will be lots of "whys" below...
The fix itself was quite small. However, an attempt to write a reasonable test case revealed a few other bugs. I just tried to fixing everything in a single patch. Okey. Let's do everything separately. These are separate reports for the problems found during writing a test case for 4634: https://mariadb.atlassian.net/browse/MDEV-4652 https://mariadb.atlassian.net/browse/MDEV-4653 https://mariadb.atlassian.net/browse/MDEV-4654 Here's a new one, found today: https://mariadb.atlassian.net/browse/MDEV-4651
On Jun 11, Alexander Barkov wrote:
=== modified file 'sql/item.cc' --- sql/item.cc 2013-03-17 06:41:22 +0000 +++ sql/item.cc 2013-06-11 11:52:43 +0000 @@ -252,7 +252,8 @@ String *Item::val_string_from_decimal(St String *Item::val_string_from_date(String *str) { MYSQL_TIME ltime; - if (get_date(<ime, TIME_FUZZY_DATE) || + uint time_flag= field_type() == MYSQL_TYPE_TIME ? TIME_TIME_ONLY : 0; + if (get_date(<ime, TIME_FUZZY_DATE | time_flag) ||
Why?
str->alloc(MAX_DATE_STRING_REP_LENGTH)) { null_value= 1;
=== modified file 'sql/item_func.cc' --- sql/item_func.cc 2013-04-06 13:14:46 +0000 +++ sql/item_func.cc 2013-06-11 10:54:50 +0000 @@ -2456,7 +2456,8 @@ bool Item_func_min_max::get_date(MYSQL_T { Item **arg= args + i; bool is_null; - longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates, &is_null); + longlong res= get_datetime_value(thd, &arg, 0, compare_as_dates, + &is_null, fuzzy_date);
why is that? to handle CONVERT_TZ(GREATEST(...)) ? That is to pass NO_ZERO_DATE down?
I'm not sure it's a good idea. Strictly speaking, only the greatest value should be NO_ZERO_DATE, not every single one.
I'd suggest to add check_date in Item_func_min_max::get_date() instead.
/* Check if we need to stop (because of error or KILL) and stop the loop */ if (thd->is_error() || args[i]->null_value) @@ -2469,6 +2470,11 @@ bool Item_func_min_max::get_date(MYSQL_T min_max= res; } unpack_time(min_max, ltime); + + if (!(fuzzy_date & TIME_TIME_ONLY) && + ((null_value= check_date_with_warn(ltime, fuzzy_date)))) + return true;
Yes, that's what I mean. If you check the return value anyway, there's no need to check it inside get_datetime_value().
if (compare_as_dates->field_type() == MYSQL_TYPE_DATE) { ltime->time_type= MYSQL_TIMESTAMP_DATE;
=== modified file 'sql/item_timefunc.cc' --- sql/item_timefunc.cc 2013-03-20 15:13:00 +0000 +++ sql/item_timefunc.cc 2013-06-11 10:57:09 +0000 @@ -1923,8 +1921,10 @@ void Item_date_add_interval::fix_length_ bool Item_date_add_interval::get_date(MYSQL_TIME *ltime, uint fuzzy_date) { INTERVAL interval; - - if (args[0]->get_date(ltime, TIME_NO_ZERO_DATE | TIME_FUZZY_DATE | TIME_NO_ZERO_IN_DATE) || + if (args[0]->get_date(ltime, + TIME_NO_ZERO_DATE | TIME_FUZZY_DATE | TIME_NO_ZERO_IN_DATE | + ((args[0]->field_type() == MYSQL_TYPE_TIME) ? + TIME_TIME_ONLY : 0)) ||
why is that?
get_interval_value(args[1], int_type, &value, &interval)) return (null_value=1);
@@ -2290,6 +2290,12 @@ bool Item_time_typecast::get_date(MYSQL_ return 1; if (decimals < TIME_SECOND_PART_DIGITS) ltime->second_part= sec_part_truncate(ltime->second_part, decimals); + if (!(fuzzy_date & TIME_TIME_ONLY)) + { + if (ltime->time_type == MYSQL_TIMESTAMP_TIME) + ltime->time_type= MYSQL_TIMESTAMP_DATETIME;
why?
+ return ((null_value= check_date_with_warn(ltime, fuzzy_date))); + } /* MYSQL_TIMESTAMP_TIME value can have non-zero day part, which we should not lose. === modified file 'sql/item_timefunc.h' --- sql/item_timefunc.h 2012-12-28 12:41:46 +0000 +++ sql/item_timefunc.h 2013-06-11 11:19:03 +0000 @@ -492,6 +492,8 @@ class Item_timefunc :public Item_tempora Item_timefunc(Item *a,Item *b) :Item_temporal_func(a,b) {} Item_timefunc(Item *a, Item *b, Item *c) :Item_temporal_func(a, b ,c) {} enum_field_types field_type() const { return MYSQL_TYPE_TIME; } + int save_in_field(Field *field, bool no_conversions) + { return save_time_in_field(field); }
why?
};
=== modified file 'sql/time.cc' --- sql/time.cc 2013-01-08 20:23:03 +0000 +++ sql/time.cc 2013-06-11 10:51:31 +0000 @@ -213,6 +213,20 @@ ulong convert_month_to_period(ulong mont }
+bool +check_date_with_warn(const MYSQL_TIME *ltime, uint fuzzy_date) +{ + int dummy_warnings; + if (check_date(ltime, fuzzy_date, &dummy_warnings)) + { + Lazy_string_time str(ltime); + make_truncated_value_warning(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, + &str, MYSQL_TIMESTAMP_ERROR, 0);
please, use this function also in Item_date_typecast::get_date() and may be in other places. you might need to pass MYSQL_TIMESTAMP_xxx value as an argument, though.
+ return true; + } + return false; +} + /* Convert a timestamp string to a MYSQL_TIME value and produce a warning if string was truncated during conversion.
Regards, Sergei