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