Hello Sergei, Thanks for your review. Please find the second version attached. Note, I also added the old behaviour with "mysqld --old" (as Monty requested). My comments are inline: On 03/06/2014 02:18 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 04, Alexander Barkov wrote:
Hi Sergei,
(my mailer failed during the previous attempt. writing again, sorry if you get this twice)
No problem. I did get it twice, but only the second copy was cc: to the mailing list, that's why I'm replying to it.
Please review the patch implementing the standard (and MySQL-5.6) compatible way of time to datetime conversion.
Here, I have only very few suggestions, but many questions :) see below
=== modified file 'include/my_time.h' --- include/my_time.h 2014-02-19 10:05:15 +0000 +++ include/my_time.h 2014-03-04 13:04:26 +0000 @@ -63,6 +63,7 @@ extern uchar days_in_month[]; #define TIME_FUZZY_DATES 1 #define TIME_DATETIME_ONLY 2 #define TIME_TIME_ONLY 4 +#define TIME_TIME_POSSIBLY 8
Why do you need "possibly"? One could think that if neither TIME_DATETIME_ONLY nor TIME_TIME_ONLY is set, then it is "possibly" either one.
Right. It works fine without adding TIME_TIME_POSSIBLY. Now check_date() just returns FALSE if time_type is MYSQL_TIMESTAMP_TIME. Thanks for the suggestion.
#define TIME_NO_ZERO_IN_DATE (1UL << 23) /* == MODE_NO_ZERO_IN_DATE */ #define TIME_NO_ZERO_DATE (1UL << 24) /* == MODE_NO_ZERO_DATE */ #define TIME_INVALID_DATES (1UL << 25) /* == MODE_INVALID_DATES */ @@ -77,6 +78,9 @@ extern uchar days_in_month[]; #define MYSQL_TIME_WARN_HAVE_WARNINGS(x) MY_TEST((x) & MYSQL_TIME_WARN_WARNINGS) #define MYSQL_TIME_WARN_HAVE_NOTES(x) MY_TEST((x) & MYSQL_TIME_WARN_NOTES)
+/* Usefull constants */ +#define SECONDS_IN_24H 86400L
in a separate follow-up changeset, please replace all instances of 86400 with SECONDS_IN_24H (althought I'd probably use SECONDS_IN_DAY)
I replaced more instances of 86400 right now, so no separate change is needed any more. This constant already exists in MySQL-5.6, so I just reused the name.
+ /* Limits for the TIME data type */ #define TIME_MAX_HOUR 838 #define TIME_MAX_MINUTE 59
=== modified file 'mysql-test/r/func_time.result' --- mysql-test/r/func_time.result 2014-02-04 09:49:44 +0000 +++ mysql-test/r/func_time.result 2014-03-04 13:13:27 +0000 @@ -2361,7 +2368,7 @@ HOUR(TIME('1 02:00:00')) HOUR(TIME('26:0 26 26 SELECT DAY(TIME('1 02:00:00')), DAY(TIME('26:00:00')); DAY(TIME('1 02:00:00')) DAY(TIME('26:00:00')) -0 0 +4 4
because 'DAY' is simply another name for 'DAYOFMONTH' and now different from EXTRACT(DAY ...) I see.
Correct.
SELECT EXTRACT(HOUR FROM '1 02:00:00'), EXTRACT(HOUR FROM '26:00:00'); EXTRACT(HOUR FROM '1 02:00:00') EXTRACT(HOUR FROM '26:00:00') 2 2 === modified file 'sql-common/my_time.c' --- sql-common/my_time.c 2013-12-16 12:02:21 +0000 +++ sql-common/my_time.c 2014-03-04 13:05:54 +0000 @@ -81,6 +81,9 @@ uint calc_days_in_year(uint year) my_bool check_date(const MYSQL_TIME *ltime, my_bool not_zero_date, ulonglong flags, int *was_cut) { + if ((flags & TIME_TIME_POSSIBLY) && + ltime->time_type == MYSQL_TIMESTAMP_TIME) + return FALSE;
okay, so you don't trust ltime->time_type anymore? why?
Now I do :)
if (not_zero_date) { if (((flags & TIME_NO_ZERO_IN_DATE) &&
=== modified file 'sql/item.cc' --- sql/item.cc 2014-02-28 09:00:31 +0000 +++ sql/item.cc 2014-03-04 13:27:37 +0000 @@ -235,6 +235,25 @@ bool Item::val_bool()
/* + Get date/time/datetime. + Optionally extended TIME result to DATETIME. +*/ +bool Item::get_date_date(MYSQL_TIME *ltime, ulonglong fuzzydate)
I don't like get_date_date name, can you find a better one, please?
This is the most difficult part :) Can you suggest please? get_temporal_not_time() does not sound well.
Why did you need a new function, shouldn't get_date() be doing it?
get_date() is not enough any more. get_date() returns in "native" format by default, and I don't change this, because some temporal hybrid functions like ADDTIME use this: mysql> select addtime('2001-01-01 00:00:00',10) as dt, addtime('10:10:10',10) as t; +---------------------+----------+ | dt | t | +---------------------+----------+ | 2001-01-01 00:00:10 | 10:10:20 | +---------------------+----------+ 1 row in set (0.00 sec) EXTRACT also wants the native format. So we have to have two separate functions, with and without TIME->DATETIME conversion.
+{ + if (get_date(ltime, fuzzydate | TIME_TIME_POSSIBLY)) + return true; + if (ltime->time_type == MYSQL_TIMESTAMP_TIME && + !(fuzzydate & TIME_TIME_ONLY)) + { + MYSQL_TIME tmp; + time_to_datetime(current_thd, ltime, &tmp); + *ltime= tmp; + } + return false; +} + + +/* For the items which don't have its own fast val_str_ascii() implementation we provide a generic slower version, which converts from the Item character set to ASCII. === modified file 'sql/item_timefunc.cc' --- sql/item_timefunc.cc 2014-02-19 10:05:15 +0000 +++ sql/item_timefunc.cc 2014-03-04 12:30:59 +0000 @@ -780,7 +780,7 @@ longlong Item_func_to_seconds::val_int_e longlong seconds; longlong days; int dummy; /* unused */ - if (get_arg0_date(<ime, TIME_FUZZY_DATES)) + if (get_arg0_date(<ime, TIME_FUZZY_DATES | TIME_INVALID_DATES))
invalid dates? for TO_SECONDS?
Thank for noticing this. There are remainders from my attempts to fix this bug at once: MDEV-5801 Partitioning does not work with 'ALLOW_INVALID_DATES' I reverted these lines in val_int_endpoint(). Btw, they did not affect anything actually, as they are followed by check_date() anyway :)
{ /* got NULL, leave the incl_endp intact */ return LONGLONG_MIN; @@ -858,7 +858,7 @@ longlong Item_func_to_days::val_int_endp MYSQL_TIME ltime; longlong res; int dummy; /* unused */ - if (get_arg0_date(<ime, 0)) + if (get_arg0_date(<ime, TIME_FUZZY_DATES | TIME_INVALID_DATES))
invalid dates? for TO_DAYS?
{ /* got NULL, leave the incl_endp intact */ return LONGLONG_MIN; @@ -2512,26 +2512,7 @@ bool Item_datetime_typecast::get_date(MY if (decimals < TIME_SECOND_PART_DIGITS) my_time_trunc(ltime, decimals);
- /* - ltime is valid MYSQL_TYPE_TIME (according to fuzzy_date). - But not every valid TIME value is a valid DATETIME value! - */ - if (ltime->time_type == MYSQL_TIMESTAMP_TIME) - { - if (ltime->neg) - { - ErrConvTime str(ltime); - make_truncated_value_warning(current_thd, Sql_condition::WARN_LEVEL_WARN, - &str, MYSQL_TIMESTAMP_DATETIME, 0); - return (null_value= 1); - } - - uint day= ltime->hour/24; - ltime->hour %= 24; - ltime->month= day / 31; - ltime->day= day % 31; - } - + DBUG_ASSERT(ltime->time_type != MYSQL_TIMESTAMP_TIME);
why?
Because get_arg0_date() calls get_date_date(), which converts TIME to DATETIME.
ltime->time_type= MYSQL_TIMESTAMP_DATETIME; return 0; } === modified file 'sql/sql_time.cc' --- sql/sql_time.cc 2014-02-19 10:05:15 +0000 +++ sql/sql_time.cc 2014-03-04 13:26:07 +0000 @@ -1133,3 +1133,88 @@ void time_to_daytime_interval(MYSQL_TIME ltime->hour%= 24; ltime->time_type= MYSQL_TIMESTAMP_NONE; } + + +/*** Conversion from TIME to DATETIME ***/ + +/* + Simple case: TIME is within normal 24 hours internal. + Mix DATE part of ldate and TIME part of ltime together. +*/ +static void +mix_date_and_time_simple(MYSQL_TIME *ldate, const MYSQL_TIME *ltime) +{ + DBUG_ASSERT(ldate->time_type == MYSQL_TIMESTAMP_DATE || + ldate->time_type == MYSQL_TIMESTAMP_DATETIME); + ldate->hour= ltime->hour; + ldate->minute= ltime->minute; + ldate->second= ltime->second; + ldate->second_part= ltime->second_part; + ldate->time_type= MYSQL_TIMESTAMP_DATETIME; +} + + +/* + Complex case: TIME is negative or outside of the 24 hour interval. +*/ +static void +mix_date_and_time_complex(MYSQL_TIME *ldate, const MYSQL_TIME *ltime) +{ + DBUG_ASSERT(ldate->time_type == MYSQL_TIMESTAMP_DATE || + ldate->time_type == MYSQL_TIMESTAMP_DATETIME); + longlong seconds; + long days, useconds; + int sign= ltime->neg ? 1 : -1; + ldate->neg= calc_time_diff(ldate, ltime, sign, &seconds, &useconds); + + DBUG_ASSERT(!ldate->neg); + DBUG_ASSERT(ldate->year > 0); + + days= (long) (seconds / SECONDS_IN_24H); + calc_time_from_sec(ldate, seconds % SECONDS_IN_24H, useconds); + get_date_from_daynr(days, &ldate->year, &ldate->month, &ldate->day); + ldate->time_type= MYSQL_TIMESTAMP_DATETIME; +}
what does sql standard says about it? where does it say, btw, about using the current date at all?
I added excerpts into: https://mariadb.atlassian.net/browse/MDEV-5372
+ + +/** + Mix a date value and a time value. + + @param IN/OUT ldate Date value. + @param ltime Time value. +*/ +static void +mix_date_and_time(MYSQL_TIME *to, const MYSQL_TIME *from) +{ + if (!from->neg && from->hour < 24) + mix_date_and_time_simple(to, from); + else + mix_date_and_time_complex(to, from); +} + + +/** + Get current date in DATE format +*/ +static void +set_current_date(THD *thd, MYSQL_TIME *to) +{ + thd->variables.time_zone->gmt_sec_to_TIME(to, thd->query_start()); + thd->time_zone_used= 1; + datetime_to_date(to); +} + + +/** + Convert time to datetime. + + The time value is added to the current datetime value. + @param IN ltime Time value to convert from. + @param OUT ltime2 Datetime value to convert to. +*/ +void +time_to_datetime(THD *thd, const MYSQL_TIME *from, MYSQL_TIME *to) +{ + set_current_date(thd, to); + mix_date_and_time(to, from); +}
=== modified file 'sql/sql_time.h' --- sql/sql_time.h 2014-02-03 14:22:39 +0000 +++ sql/sql_time.h 2014-03-04 03:17:57 +0000 @@ -49,6 +49,21 @@ bool int_to_datetime_with_warn(longlong ulonglong fuzzydate, const char *name);
+void time_to_datetime(THD *thd, const MYSQL_TIME *tm, MYSQL_TIME *dt); +inline void datetime_to_time(MYSQL_TIME *ltime) +{ + ltime->year= ltime->month= ltime->day= 0; + ltime->time_type= MYSQL_TIMESTAMP_TIME; +} +inline void datetime_to_date(MYSQL_TIME *ltime) +{
here and below. I'd added asserts for incoming ltime->time_type like DBUG_ASSERT(ltime->time_type == MYSQL_TIMESTAMP_DATETIME)
Probably these are bad function names. They are not actually restricted to the source type. Should I remove the functions to just: to_time() to_date() to_datetime() ? (I copied these functions form MySQL-5.6)
+ ltime->hour= ltime->minute= ltime->second= ltime->second_part= 0; + ltime->time_type= MYSQL_TIMESTAMP_DATE; +} +inline void date_to_datetime(MYSQL_TIME *ltime) +{
DBUG_ASSERT(ltime->time_type == MYSQL_TIMESTAMP_DATE)
+ ltime->time_type= MYSQL_TIMESTAMP_DATETIME; +} void make_truncated_value_warning(THD *thd, Sql_condition::enum_warning_level level, const ErrConv *str_val,
Regards, Sergei