[Maria-developers] Standard way for time->datetime cast
Hi Sergei, (my mailer failed during the previous attempt. writing again, sorry if you get this twice) Please review the patch implementing the standard (and MySQL-5.6) compatible way of time to datetime conversion. Thanks.
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.
#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)
+ /* 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.
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?
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? Why did you need a new function, shouldn't get_date() be doing it?
+{ + 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?
{ /* 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?
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?
+ + +/** + 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)
+ 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
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
Hi, Alexander! On Mar 06, Alexander Barkov wrote:
+/* 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.
I remember when I grepped, there was one instance on 86400000 (microseconds, I guess). That's why I wrote above "86400" and not "86400L" :)
This constant already exists in MySQL-5.6, so I just reused the name.
Okay, that's a good reason.
=== 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 === 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: ... EXTRACT also wants the native format.
Yes, but we already have flags for that. TIME_DATETIME_ONLY only means that you need to convert to datetime. If there is no TIME_DATETIME_ONLY, then you return the native format. It's only if (fuzzydate & TIME_DATETIME_ONLY && ltime->type == MYSQL_TIMESTAMP_TIME) { MYSQL_TIME tmp; time_to_datetime(current_thd, ltime, &tmp); *ltime= tmp; } On the other hand... get_date() is virtual, so we'd need this code in every get_date() implementation... So, perhaps non-virtual wrapper would be good, okay. A name could be get_date_with_conversion(). We already have set_field_to_null_with_conversions, fetch_datetime_with_conversion, etc.
=== 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
Right, it describes the conversion between TIME and DATETIME, where both are, as the standard calls them, "datetime types". See 4.6.2 Datetimes. It also says that The <primary datetime field>s other than SECOND contain non-negative integer values, constrained by the natural rules for dates using the Gregorian calendar. SECOND, however, can be defined to have a <time fractional seconds precision> Here <primary datetime field> is one of YEAR, MONTH, DAY, HOUR, MINUTE, SECOND. Anyway, this means that a TIME that can be casted to a DATETIME must be between 00:00:00 and 23:59:59.999999 Our TIME type is more like an interval type from the standard, and acording to the standard one cannot cast an interval to a datetime, it's not allowed. Anyway, I think your implementation is ok (I just wanted to know whether it's the standard behavior or an extension), but asserts might be not. What if someone does SET TIMESTAMP='0000-00-00 00:00:01' ?
=== 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 rename the functions to just:
to_time() to_date() to_datetime()
Not really, they're exactly datetime_to_date() and date_to_datetime(). See, I didn't comment on time_to_datetime(), I didn't say you need an assert there. Because any time could be converted to MYSQL_TIMESTAMP_TIME with that function. But datetime_to_date() simply sets the type, so if you'd pass MYSQL_TIMESTAMP_TIME as input, the result will be 0000-00-00, not CURRENT_DATE(). I know that it's a low-level function that shouldn't use current_thd, but it would be confusing to have two ways of casting TIME to DATE. Better to limit datetime_to_date() to DATETIME type only. I mean, "limit" (in quotes), because it actually doesn't limit anything, you don't use it for MYSQL_TIMESTAMP_TIME anyway.
=== modified file 'sql/item.cc' --- sql/item.cc 2014-02-28 09:00:31 +0000 +++ sql/item.cc 2014-03-06 10:22:00 +0000 @@ -235,6 +235,50 @@ bool Item::val_bool()
/* + Get date/time/datetime. + Optionally extend TIME result to DATETIME. +*/ +bool Item::get_date_date(MYSQL_TIME *ltime, ulonglong fuzzydate) +{ + /* + Some TIME type items return error when trying to do get_date() + without TIME_TIME_ONLY set (e.g. Item_field for Field_time). + In the SQL standard time->datetime conversion mode we add TIME_TIME_ONLY. + In the legacy time->datetime conversion mode we let the Item to check date, + like it was before. + */ + ulonglong time_flag= (field_type() == MYSQL_TYPE_TIME && + !current_thd->variables.old_mode) ? TIME_TIME_ONLY : 0;
please, only do one current_thd per function
+ if (get_date(ltime, fuzzydate | time_flag)) + return true; + if (ltime->time_type == MYSQL_TIMESTAMP_TIME && + !(fuzzydate & TIME_TIME_ONLY)) + { + MYSQL_TIME tmp; + THD *thd= current_thd; + int warn= 0; + /* + After time_to_datetime() we need to do check_date(), as + the caller may want TIME_NO_ZERO_DATE or TIME_NO_ZERO_IN_DATE. + Note, the SQL standard time->datetime conversion mode always returns + a valid date based on CURRENT_DATE. So we need to do check_date() + only in the old mode. + */ + if (time_to_datetime(thd, ltime, &tmp) || + (thd->variables.old_mode && check_date(&tmp, fuzzydate, &warn))) + { + ErrConvTime str(ltime); + make_truncated_value_warning(thd, Sql_condition::WARN_LEVEL_WARN, + &str, MYSQL_TIMESTAMP_DATETIME, 0); + return true; + } + *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.
Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik