Hi Sergei, On 06/28/2013 07:23 PM, Sergei Golubchik wrote:

Hi, Alexander!

On Jun 26, Alexander Barkov wrote:

...

Hi Serg,

Please review the patch merging MySQL-5.6 temporal literals.

Here is is, below:

Thanks for your review. I addressed most of your comments. The latest patch is attached. Please see the details inline:

...

=== modified file 'client/mysqlbinlog.cc' --- client/mysqlbinlog.cc 2013-04-15 13:09:22 +0000 +++ client/mysqlbinlog.cc 2013-06-21 13:31:28 +0000 @@ -1541,13 +1541,14 @@ the mysql command line client.\n\n");

static my_time_t convert_str_to_timestamp(const char* str) { - int was_cut; + MYSQL_TIME_STATUS status; MYSQL_TIME l_time; long dummy_my_timezone; uint dummy_in_dst_time_gap; + /* We require a total specification (date AND time) */ - if (str_to_datetime(str, (uint) strlen(str), &l_time, 0, &was_cut) != - MYSQL_TIMESTAMP_DATETIME || was_cut) + if (str_to_datetime(str, (uint) strlen(str), &l_time, 0, &status) || + l_time.time_type != MYSQL_TIMESTAMP_DATETIME || status.warnings)

Isn't it better to return l_time.time_type from str_to_datetime? otherwise you'll always need to write like the above

if (str_to_datetime(...) || l_time.time_type != ...)

Let's see all the calls for str_to_datetime() and str_to_time(): - The cases that do not need timestamp type (only a boolean result is important): field.cc: have_smth_to_conv= !str_to_datetime(cs, from, len, &l_time, field.cc: bool func_res= !str_to_datetime(cs, from, len, <ime, field.cc: !str_to_time(cs, from, len, <ime, sql_time.cc: bool ret_val= str_to_datetime(cs, str, length, l_time, (the code has reduced here after switch to boolean result type) - The cases where timestamp type is needed: item_create.cc: if (!str_to_datetime(cs, str, length, <ime, flags, &status) && item_create.cc: if (!str_to_datetime(cs, str, length, <ime, flags, &status) && item_create.cc: if (!str_to_time(cs, str, length, <ime, 0, &status) && (the new code creating temporal literals) - The cases where timestamp type in needed only in DBUG_ASSERT: item_cmpfunc.cc: DBUG_ASSERT(l_time->time_type == MYSQL_TIMESTAMP_DATETIME || l_time->time_type == MYSQL_TIMESTAMP_DATE); item_cmpfunc.cc: DBUG_ASSERT(l_time->time_type != MYSQL_TIMESTAMP_TIME); (in case of a timestamp_type result, a variable of timestamp_type would be needed, but only in debug build, which is inconvenient). Note, also the boolean result is inline with str_to_datetime_with_warn(). It's easier to remember that way. I think a boolean result is more convenient.

...

{ error("Incorrect date and time argument: %s", str); exit(1); === modified file 'include/my_time.h' --- include/my_time.h 2013-05-24 15:09:59 +0000 +++ include/my_time.h 2013-06-25 06:28:08 +0000 @@ -78,14 +78,27 @@ extern uchar days_in_month[]; #define TIME_MAX_VALUE_SECONDS (TIME_MAX_HOUR * 3600L + \ TIME_MAX_MINUTE * 60L + TIME_MAX_SECOND)

+/* + Structure to return status from + str_to_datetime(), str_to_time(), number_to_datetime(), number_to_time() +*/ +typedef struct st_mysql_time_status +{ + int warnings; + uint fractional_digits;

could you please rename it to "precision" ? or at least, add a comment explaining what it contains

Renamed to "precision".

...

+} MYSQL_TIME_STATUS; + +static inline void my_time_status_init(MYSQL_TIME_STATUS *status) +{ + status->warnings= status->fractional_digits= 0; +} + my_bool check_date(const MYSQL_TIME *ltime, my_bool not_zero_date, ulonglong flags, int *was_cut); -enum enum_mysql_timestamp_type -str_to_time(const char *str, uint length, MYSQL_TIME *l_time, - ulonglong flag, int *warning); -enum enum_mysql_timestamp_type -str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time, - ulonglong flags, int *was_cut); +my_bool str_to_time(const char *str, uint length, MYSQL_TIME *l_time, + ulonglong flag, MYSQL_TIME_STATUS *status); +my_bool str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time, + ulonglong flags, MYSQL_TIME_STATUS *status); longlong number_to_datetime(longlong nr, ulong sec_part, MYSQL_TIME *time_res, ulonglong flags, int *was_cut);

=== added file 'mysql-test/r/temporal_literal.result' --- mysql-test/r/temporal_literal.result 1970-01-01 00:00:00 +0000 +++ mysql-test/r/temporal_literal.result 2013-06-26 09:12:13 +0000 @@ -0,0 +1,412 @@ +DROP TABLE IF EXISTS t1, t2; +SET NAMES latin1; +# +# Testing DATE literals +# +SELECT DATE'xxxx'; +ERROR HY000: Incorrect DATE value: 'xxxx' +SELECT DATE'01'; +ERROR HY000: Incorrect DATE value: '01' +SELECT DATE'01-01'; +ERROR HY000: Incorrect DATE value: '01-01' +SELECT DATE'2001'; +ERROR HY000: Incorrect DATE value: '2001' +SELECT DATE'2001-01'; +ERROR HY000: Incorrect DATE value: '2001-01' +SELECT DATE'2001-00-00'; +DATE'2001-00-00' +2001-00-00 +SELECT DATE'2001-01-00'; +DATE'2001-01-00' +2001-01-00 +SELECT DATE'0000-00-00'; +DATE'0000-00-00' +0000-00-00 +SELECT DATE'2001-01-01 00:00:00'; +ERROR HY000: Incorrect DATE value: '2001-01-01 00:00:00' +SELECT DATE'01:01:01'; +DATE'01:01:01' +2001-01-01 +SELECT DATE'01-01-01'; +DATE'01-01-01' +2001-01-01 +SELECT DATE'2010-01-01'; +DATE'2010-01-01' +2010-01-01 +SELECT DATE '2010-01-01'; +DATE '2010-01-01' +2010-01-01 +CREATE TABLE t1 AS SELECT DATE'2010-01-01'; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `DATE'2010-01-01'` date NOT NULL DEFAULT '0000-00-00' +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +DROP TABLE t1; +CREATE TABLE t1 AS SELECT +{d'2001-01-01'}, +{d'2001-01-01 10:10:10'};

This (the second value, I mean) is what, backward compatibility?

These are ODBC escape sequences. http://msdn.microsoft.com/en-us/library/ms190234%28v=sql.90%29.aspx IIRC, applications like MSQuery and MSAccess use this format.

...

+SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `{d'2001-01-01'}` date NOT NULL DEFAULT '0000-00-00', + `2001-01-01 10:10:10` varchar(19) NOT NULL DEFAULT '' +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +DROP TABLE t1; +EXPLAIN EXTENDED SELECT {d'2010-01-01'}; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL NULL No tables used +Warnings: +Note 1003 select DATE'2010-01-01' AS `{d'2010-01-01'}` +EXPLAIN EXTENDED SELECT DATE'2010-01-01'; +id select_type table type possible_keys key key_len ref rows filtered Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL NULL No tables used +Warnings: +Note 1003 select DATE'2010-01-01' AS `DATE'2010-01-01'` ... === modified file 'sql-common/my_time.c' --- sql-common/my_time.c 2013-04-07 12:00:16 +0000 +++ sql-common/my_time.c 2013-06-26 09:23:23 +0000 @@ -230,20 +229,43 @@ static uint skip_digits(const char **str

The second part may have an optional .###### fraction part.

- RETURN VALUES + NOTES + This function should work with a format position vector as long as the + following things holds: + - All date are kept together and all time parts are kept together + - Date and time parts must be separated by blank + - Second fractions must come after second part and be separated + by a '.'. (The second fractions are optional) + - AM/PM must come after second fractions (or after seconds if no fractions) + - Year must always been specified. + - If time is before date, then we will use datetime format only if + the argument consist of two parts, separated by space. + Otherwise we will assume the argument is a date. + - The hour part must be specified in hour-minute-second order.

I've removed this comment (NOTES) from when I was adding skip_digits() and other small helpers. Simply because it was mostly wrong. It's ok to add it back, if you'd like to, but then, please fix to to reflect the reality (and fix the language too :)

on the other hand, feel free to remove it again, if you want. the code is pretty much self-documenting now.

Removed the comment.

...

+ + status->warnings is set to: + 0 Value OK + MYSQL_TIME_WARN_TRUNCATED If value was cut during conversion + MYSQL_TIME_WARN_OUT_OF_RANGE check_date(date,flags) considers date invalid + + l_time->time_type is set as follows: MYSQL_TIMESTAMP_NONE String wasn't a timestamp, like [DD [HH:[MM:[SS]]]].fraction. + l_time is not changed. MYSQL_TIMESTAMP_DATE DATE string (YY MM and DD parts ok) MYSQL_TIMESTAMP_DATETIME Full timestamp MYSQL_TIMESTAMP_ERROR Timestamp with wrong values. All elements in l_time is set to 0 + RETURN VALUES + 0 - Ok + 1 - Error */

#define MAX_DATE_PARTS 8

-enum enum_mysql_timestamp_type +my_bool str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time, - ulonglong flags, int *was_cut) + ulonglong flags, MYSQL_TIME_STATUS *status) { const char *end=str+length, *pos; uint number_of_fields= 0, digits, year_length, not_zero_date; @@ -513,12 +538,26 @@ str_to_time(const char *str, uint length if (field_length-- > 0) value= value*10 + (uint) (uchar) (*str - '0'); } - if (field_length > 0) - value*= (long) log_10_int[field_length]; - else if (field_length < 0) - *warning|= MYSQL_TIME_WARN_TRUNCATED; + if (field_length >= 0) + { + status->fractional_digits= TIME_SECOND_PART_DIGITS - field_length; + if (field_length > 0) + value*= (long) log_10_int[field_length]; + } + else + { + /* Scan digits left after microseconds */ + status->fractional_digits= 6; + for ( ; str != end && my_isdigit(&my_charset_latin1, *str); str++)

1. old code set MYSQL_TIME_WARN_TRUNCATED, you don't do that anymore

Thanks for noticing this. This did not affect the tests, but anyway, I slightly modified the logic of str_to_time() to be closer to the one in str_to_datetime(): A label "err" in the end, which sets time_type to MYSQL_TIMESTAMP_ERROR and returns TRUE. The warning is now set before all "goto err".

2. there's skip_digits() helper, why not to use it?

Done.

3. actually the whole loop can be probaby replaced by get_digits() call

Which loop do you mean?

...

+ { } + } date[4]= (ulong) value; } + else if ((end - str) == 1 && *str == '.') + { + str++; + date[4]= 0;

is it for, like, "10:10:10." ?

...

+ } else date[4]=0;

@@ -553,7 +592,7 @@ str_to_time(const char *str, uint length if (date[0] > UINT_MAX || date[1] > UINT_MAX || date[2] > UINT_MAX || date[3] > UINT_MAX || date[4] > UINT_MAX) - return MYSQL_TIMESTAMP_ERROR; + return TRUE;

you need to set l_time->type= MYSQL_TIMESTAMP_ERROR (because you no longer return it). Probably better to do it unconditionally, early in str_to_time().

Done.

...

l_time->year= 0; /* For protocol::store_time */ l_time->month= 0; === modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2013-06-06 15:51:28 +0000 +++ sql/item_cmpfunc.cc 2013-06-26 08:12:33 +0000 @@ -721,19 +721,18 @@ bool get_mysql_time_from_str(THD *thd, S const char *warn_name, MYSQL_TIME *l_time) { bool value; - int error; - enum_mysql_timestamp_type timestamp_type; + MYSQL_TIME_STATUS status; int flags= TIME_FUZZY_DATE | MODE_INVALID_DATES; ErrConvString err(str);

+ DBUG_ASSERT(warn_type != MYSQL_TIMESTAMP_TIME); // Serg: why the below code?

I don't remember. Currently warn_type can never be MYSQL_TIMESTAMP_TIME here. This function is only called from stored_field_cmp_to_item() which is a historical hack and should go away completely. Together with this get_mysql_time_from_str()

Ok. Let's have this DBUG_ASSERT then. I also added two other DBUG_ASSERTs in this function, to make sure we never get TIME here.

...

if (warn_type == MYSQL_TIMESTAMP_TIME) flags|= TIME_TIME_ONLY;

- timestamp_type= - str_to_datetime(str->charset(), str->ptr(), str->length(), - l_time, flags, &error); - - if (timestamp_type > MYSQL_TIMESTAMP_ERROR) + if (!str_to_datetime(str->charset(), str->ptr(), str->length(), + l_time, flags, &status) && + (l_time->time_type == MYSQL_TIMESTAMP_DATETIME || + l_time->time_type == MYSQL_TIMESTAMP_DATE)) /* Do not return yet, we may still want to throw a "trailing garbage" warning. === modified file 'sql/item_timefunc.h' --- sql/item_timefunc.h 2013-03-13 21:33:52 +0000 +++ sql/item_timefunc.h 2013-06-24 07:26:29 +0000 @@ -526,6 +526,136 @@ class Item_timefunc :public Item_tempora };

+/** + DATE'2010-01-01' +*/ +class Item_date_literal :public Item_datefunc +{ + MYSQL_TIME cached_time; +public: + /** + Constructor for Item_date_literal. + @param ltime DATE value. + */ + Item_date_literal(MYSQL_TIME *ltime) :Item_datefunc() + { + cached_time= *ltime; + fix_length_and_dec(); + fixed= 1; + } + const char *func_name() const { return "date_literal"; } + void print(String *str, enum_query_type query_type); + bool get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date); + void fix_length_and_dec() + { + Item_datefunc::fix_length_and_dec(); + set_persist_maybe_null(false); + } + bool check_partition_func_processor(uchar *int_arg) + { + return FALSE; + } + bool basic_const_item() const { return true; } + bool const_item() const { return true; } + table_map used_tables() const { return (table_map) 0L; } + void cleanup() + { + // See Item_basic_const::cleanup() + if (orig_name) + name= orig_name; + } + bool eq(const Item *item, bool binary_cmp) const; +};

Hmm. I don't really like this hierarchy. It's not very logical to derive basic const items from Item_func. And you need to overwrite lots of virtual methods. For every child too, so there's a lot of code duplication :(

Would it be better to derive these classes from Item_basic_constant?

Well, that was a choice of "less evil". I had to overwrite only these simple methods: bool check_partition_func_processor(uchar *int_arg) { return FALSE; } bool basic_const_item() const { return true; } bool const_item() const { return true; } table_map used_tables() const { return (table_map) 0L; } void cleanup() { // See Item_basic_const::cleanup() if (orig_name) name= orig_name; } If we derive from Item_basic_constant, then we'll have to overwrite more methods: enum Item_result result_type () const { return STRING_RESULT; } CHARSET_INFO *charset_for_protocol(void) const { return &my_charset_bin; } enum_field_types field_type() const { return MYSQL_TYPE_DATETIME; } Item_result cmp_type() const { return TIME_RESULT; } String *val_str(String *str) { return val_string_from_date(str); } my_decimal *val_decimal(my_decimal *decimal_value) { return val_decimal_from_date(decimal_value); } longlong val_int(); double val_real(); And further, move the code: - from Item_temporal_func::val_int() to a new method Item::val_int_from_date() - from Item_temporal_func::val_real() to new method Item::val_real_from_date() (to reuse the code in both Item_{time|datetime|date}_literal and Item_temporal_func. Looks like a much more amount of new code and amount of changes. What do you think? In long terms, I'd suggest to consider multiple inheritance here. The methods related to constant/non-constant processing could be collected into a separate class inside Item.

...

+ + +/** + TIME'10:10:10' +*/ +class Item_time_literal :public Item_timefunc +{ + MYSQL_TIME cached_time; === modified file 'sql/sql_yacc.yy' --- sql/sql_yacc.yy 2013-06-06 19:32:29 +0000 +++ sql/sql_yacc.yy 2013-06-20 11:21:37 +0000 @@ -8741,7 +8742,49 @@ simple_expr: MYSQL_YYABORT; } | '{' ident expr '}' - { $$= $3; } + { + Item_string *item; + $$= NULL; + /* + If "expr" is reasonably short pure ASCII string literal, + try to parse known ODBC style date, time or timestamp literals, + e.g: + SELECT {d'2001-01-01'}; + SELECT {t'10:20:30'}; + SELECT {ts'2001-01-01 10:20:30'}; + */ + if ($3->type() == Item::STRING_ITEM && + (item= (Item_string *) $3) && + item->collation.repertoire == MY_REPERTOIRE_ASCII && + item->str_value.length() < MAX_DATE_STRING_REP_LENGTH * 4) + { + enum_field_types type= MYSQL_TYPE_STRING; + ErrConvString str(&item->str_value);

1. it's a confusing usage of ErrConvString. this class is for error messages 2. why do you convert to system_charset_info at all here?

The token $3 is Item_string, which is created taking into account character_set_connection, which can be "tricky", e.g. ucs2, utf16, utf32. So, we need to convert the value from a potentially "tricky" charset to ASCII-compatible representation, so create_temporal_literal can understand. Monty also found this confusing when I used ErrConvString() and passed it to get_charset_by_csname() and get_charset_by_name(). See sys_vars.cc in 10.0. I agree that it might look confusing. But this is the fastest and shortest possible way to convert to an ASCII-compatible presentation. ErrConvString() perfectly suites this need. Maybe we should just rename it to ConvString or UTF8String :)

...

+ LEX_STRING *ls= &$2; + if (ls->length == 1) + { + if (ls->str[0] == 'd') /* {d'2001-01-01'} */ + type= MYSQL_TYPE_DATE; + else if (ls->str[0] == 't') /* {t'10:20:30'} */ + type= MYSQL_TYPE_TIME; + } + else if (ls->length == 2) /* {ts'2001-01-01 10:20:30'} */ + { + if (ls->str[0] == 't' && ls->str[1] == 's') + type= MYSQL_TYPE_DATETIME; + } + if (type != MYSQL_TYPE_STRING) + { + const char *ascii= str.ptr(); + $$= create_temporal_literal(YYTHD, + ascii, strlen(ascii), + system_charset_info, + type, false); + } + } + if ($$ == NULL) + $$= $3; + } | MATCH ident_list_arg AGAINST '(' bit_expr fulltext_options ')' { $2->push_front($5); Regards, Sergei