[Maria-developers] Please review a patch fixing a few MDEV-6001 blockers
Hi Sergei, can you please review my patch fixing a few bugs that are blockers for MDEV-6001? - MDEV-4858 Wrong results for a huge unsigned value inserted into a TIME column - MDEV-6097 Inconsistent results for CAST(int,decimal,double AS DATETIME) - MDEV-6099 Bad results for DATE_ADD(.., INTERVAL 2000000000000000000.0 SECOND) - MDEV-6100 No warning on CAST(9000000 AS TIME) - MDEV-6101 Hybrid functions do not add CURRENT_DATE when converting TIME to DATETIME - MDEV-6102 Comparison between TIME and DATETIME does not use CURRENT_DATE Thanks.
Hi, Alexander! On Apr 15, Alexander Barkov wrote:
Hi Sergei,
can you please review my patch fixing a few bugs that are blockers for MDEV-6001?
- MDEV-4858 Wrong results for a huge unsigned value inserted into a TIME column - MDEV-6097 Inconsistent results for CAST(int,decimal,double AS DATETIME) - MDEV-6099 Bad results for DATE_ADD(.., INTERVAL 2000000000000000000.0 SECOND) - MDEV-6100 No warning on CAST(9000000 AS TIME) - MDEV-6101 Hybrid functions do not add CURRENT_DATE when converting TIME to DATETIME - MDEV-6102 Comparison between TIME and DATETIME does not use CURRENT_DATE
First question: One of these bugs is marked for 5.3, two - for 5.5, two for 10.0. But here they are all in one patch. Where are you going to push it? Now, the review: Code-wise everything is ok. I have one comment about the code, it could simplify the function, if I'm right. But I don't agree with some of the behavior changes that you've implemented, see below. (note that code changes that cause these behavior changes are ok - if you convince me that the new behavior is ok, this automatically means that these code changes are ok to push).
=== modified file 'include/my_time.h' --- include/my_time.h 2014-03-06 20:21:25 +0000 +++ include/my_time.h 2014-04-15 03:10:39 +0000 @@ -125,7 +125,7 @@ longlong double_to_datetime(double nr, M ltime, flags, cut); }
-int number_to_time(my_bool neg, longlong nr, ulong sec_part, +int number_to_time(my_bool neg, ulonglong nr, ulong sec_part, MYSQL_TIME *ltime, int *was_cut);
Why is that? As far as I understand, the actual fix for MDEV-4858 is inside number_to_time() and in the Field_time::store(longlong nr). And it looks like it doesn't rely on 'nr' being unsigned.
ulonglong TIME_to_ulonglong_datetime(const MYSQL_TIME *); ulonglong TIME_to_ulonglong_date(const MYSQL_TIME *);
=== modified file 'mysql-test/r/cast.result' --- mysql-test/r/cast.result 2014-03-26 21:25:38 +0000 +++ mysql-test/r/cast.result 2014-04-09 13:32:17 +0000 @@ -327,7 +327,7 @@ cast(cast(120010203101112.121314 as doub NULL select cast(cast(1.1 as double) as datetime); cast(cast(1.1 as double) as datetime) -0000-00-00 00:00:01 +NULL
Why? I'd expect 2000-00-00 00:00:01, in line with your other changes
select cast(cast(-1.1 as double) as datetime); cast(cast(-1.1 as double) as datetime) NULL
=== modified file 'mysql-test/r/dyncol.result' --- mysql-test/r/dyncol.result 2014-03-13 08:38:41 +0000 +++ mysql-test/r/dyncol.result 2014-04-15 06:42:12 +0000 @@ -1766,5 +1766,16 @@ group_concat(cast(column_json(dyn) as ch {"name1":"value1","name2":"value2"} drop table t1; # +# MDEV-4858 Wrong results for a huge unsigned value inserted into a TIME column +#
here you can add +# huge unsigned values are already tested above, let's test signed too otherwise this looks really strange, more like a typo, and someone might want to "fix" your tests.
+SELECT +column_get(column_create(1, -999999999999999 AS int), 1 AS TIME) AS t1, +column_get(column_create(1, -9223372036854775808 AS int), 1 AS TIME) AS t2; +t1 t2 +-838:59:59 -838:59:59 +Warnings: +Warning 1292 Truncated incorrect time value: '-999999999999999' +Warning 1292 Truncated incorrect time value: '-9223372036854775808' +# # end of 10.0 tests #
=== modified file 'mysql-test/r/func_time.result' --- mysql-test/r/func_time.result 2014-03-23 10:22:44 +0000 +++ mysql-test/r/func_time.result 2014-04-15 06:56:53 +0000 @@ -1091,9 +1091,9 @@ NULL select isnull(week(now() + 0)), isnull(week(now() + 0.2)), week(20061108), week(20061108.01), week(20061108085411.000002); isnull(week(now() + 0)) isnull(week(now() + 0.2)) week(20061108) week(20061108.01) week(20061108085411.000002) -0 0 45 NULL 45 +0 0 45 45 45
Why? Here you make 20061108.01 to be casted to datetime as "2006-11-06 ??:??:??" I'm not sure that's a good idea. Old code assumed that a non-zero fractional part *always* means sub-seconds, which automatically means the number should be interpreted as YYYYMMDDHHMMSS.uuuuuu, it cannot be possibly interpreted as YYYYMMDD.uuuuuu
Warnings: -Warning 1292 Incorrect datetime value: '20061108.01' +Warning 1292 Truncated incorrect datetime value: '20061108.01' End of 4.1 tests select time_format('100:00:00', '%H %k %h %I %l'); time_format('100:00:00', '%H %k %h %I %l') === modified file 'mysql-test/r/old-mode.result' --- mysql-test/r/old-mode.result 2014-03-07 20:05:28 +0000 +++ mysql-test/r/old-mode.result 2014-04-14 12:51:24 +0000 @@ -95,8 +95,9 @@ INSERT INTO t1 VALUES (NULL, '00:20:12') INSERT INTO t1 VALUES (NULL, '-00:20:12'); SELECT IF(1,ADDDATE(IFNULL(a,b),0),1) FROM t1; IF(1,ADDDATE(IFNULL(a,b),0),1) -0000-00-00 00:20:12 +NULL NULL Warnings: +Warning 1292 Incorrect datetime value: '0000-00-00 00:20:12'
Why? Because ADDDATE() requires a valid DATETIME value?
Warning 1292 Truncated incorrect datetime value: '-00:20:12' DROP TABLE t1;
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
Warning 1264 Out of range value for column 'c17' at row 1 insert into t9 ( c1, c13, c14, c15, c16, c17 ) === modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
+ return true; + default: + return false; + } +} + === modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
{ longlong UNINIT_VAR(value); Item *item= **item_arg;
Regards, Sergei
Hi Sergei, Thanks for review. Please find my comments inline: On 04/30/2014 10:49 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Apr 15, Alexander Barkov wrote:
Hi Sergei,
can you please review my patch fixing a few bugs that are blockers for MDEV-6001?
- MDEV-4858 Wrong results for a huge unsigned value inserted into a TIME column - MDEV-6097 Inconsistent results for CAST(int,decimal,double AS DATETIME) - MDEV-6099 Bad results for DATE_ADD(.., INTERVAL 2000000000000000000.0 SECOND) - MDEV-6100 No warning on CAST(9000000 AS TIME) - MDEV-6101 Hybrid functions do not add CURRENT_DATE when converting TIME to DATETIME - MDEV-6102 Comparison between TIME and DATETIME does not use CURRENT_DATE
First question:
One of these bugs is marked for 5.3, two - for 5.5, two for 10.0. But here they are all in one patch. Where are you going to push it?
I was going to push into 10.0, then backport the relevant parts into 5.3 and 5.5.
Now, the review:
Code-wise everything is ok. I have one comment about the code, it could simplify the function, if I'm right.
Which function?
But I don't agree with some of the behavior changes that you've implemented, see below. (note that code changes that cause these behavior changes are ok - if you convince me that the new behavior is ok, this automatically means that these code changes are ok to push).
=== modified file 'include/my_time.h' --- include/my_time.h 2014-03-06 20:21:25 +0000 +++ include/my_time.h 2014-04-15 03:10:39 +0000 @@ -125,7 +125,7 @@ longlong double_to_datetime(double nr, M ltime, flags, cut); }
-int number_to_time(my_bool neg, longlong nr, ulong sec_part, +int number_to_time(my_bool neg, ulonglong nr, ulong sec_part, MYSQL_TIME *ltime, int *was_cut);
Why is that? As far as I understand, the actual fix for MDEV-4858 is inside number_to_time() and in the Field_time::store(longlong nr). And it looks like it doesn't rely on 'nr' being unsigned.
Before the fix there were 3 functions with different API: int number_to_time(my_bool neg, longlong nr,...); static bool number_to_time_with_warn(bool neg, ulonglong nr, ...); bool int_to_datetime_with_warn(longlong value, ...); After the fix there are 3 functions with the same API: int number_to_time(my_bool neg, ulonglong nr, ...); static bool number_to_time_with_warn(bool neg, ulonglong nr, ...); bool int_to_datetime_with_warn(bool neg, ulonglong value, ...); I found it good. Moreover, these functions call each other and the same API helps to avoid casts. Also, there is a little sense to have both "my_bool neg" and "signed longlong" at the same time.
ulonglong TIME_to_ulonglong_datetime(const MYSQL_TIME *); ulonglong TIME_to_ulonglong_date(const MYSQL_TIME *);
=== modified file 'mysql-test/r/cast.result' --- mysql-test/r/cast.result 2014-03-26 21:25:38 +0000 +++ mysql-test/r/cast.result 2014-04-09 13:32:17 +0000 @@ -327,7 +327,7 @@ cast(cast(120010203101112.121314 as doub NULL select cast(cast(1.1 as double) as datetime); cast(cast(1.1 as double) as datetime) -0000-00-00 00:00:01 +NULL
Why? I'd expect 2000-00-00 00:00:01, in line with your other changes
select cast(cast(-1.1 as double) as datetime); cast(cast(-1.1 as double) as datetime) NULL
If I test this query: SELECT CAST(1 AS DATETIME), CAST(1.1 AS DATETIME), CAST(1.1e0 AS DATETIME); MySQL-5.6: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | NULL | NULL | +---------------------+-----------------------+-------------------------+ 1 row in set, 3 warnings (0.03 sec) Before the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | 0000-00-00 00:00:01 | 0000-00-00 00:00:01 | +---------------------+-----------------------+-------------------------+ 1 row in set, 1 warning (0.01 sec) After the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | NULL | NULL | +---------------------+-----------------------+-------------------------+ 1 row in set, 3 warnings (0.00 sec) The version "after the fix" is more consistent, and is MySQL-5.6 compatible.
=== modified file 'mysql-test/r/dyncol.result' --- mysql-test/r/dyncol.result 2014-03-13 08:38:41 +0000 +++ mysql-test/r/dyncol.result 2014-04-15 06:42:12 +0000 @@ -1766,5 +1766,16 @@ group_concat(cast(column_json(dyn) as ch {"name1":"value1","name2":"value2"} drop table t1; # +# MDEV-4858 Wrong results for a huge unsigned value inserted into a TIME column +#
here you can add
+# huge unsigned values are already tested above, let's test signed too
otherwise this looks really strange, more like a typo, and someone might want to "fix" your tests.
Thanks. Added.
+SELECT +column_get(column_create(1, -999999999999999 AS int), 1 AS TIME) AS t1, +column_get(column_create(1, -9223372036854775808 AS int), 1 AS TIME) AS t2; +t1 t2 +-838:59:59 -838:59:59 +Warnings: +Warning 1292 Truncated incorrect time value: '-999999999999999' +Warning 1292 Truncated incorrect time value: '-9223372036854775808' +# # end of 10.0 tests #
=== modified file 'mysql-test/r/func_time.result' --- mysql-test/r/func_time.result 2014-03-23 10:22:44 +0000 +++ mysql-test/r/func_time.result 2014-04-15 06:56:53 +0000 @@ -1091,9 +1091,9 @@ NULL select isnull(week(now() + 0)), isnull(week(now() + 0.2)), week(20061108), week(20061108.01), week(20061108085411.000002); isnull(week(now() + 0)) isnull(week(now() + 0.2)) week(20061108) week(20061108.01) week(20061108085411.000002) -0 0 45 NULL 45 +0 0 45 45 45
Why? Here you make 20061108.01 to be casted to datetime as "2006-11-06 ??:??:??" I'm not sure that's a good idea. Old code assumed that a non-zero fractional part *always* means sub-seconds, which automatically means the number should be interpreted as YYYYMMDDHHMMSS.uuuuuu, it cannot be possibly interpreted as YYYYMMDD.uuuuuu
Producing very different results for "CAST(20061108.01 AS DATETIME)" and "CAST(20061108.00 AS DATETIME)" looks confusing for me. I think this is the integer part who should determine how the number converts to date/datetime. Also, the version after fix is MySQL-5.6 compatible: MySQL-5.6: +-------------------------------+ | CAST(20061108.01 AS DATETIME) | +-------------------------------+ | 2006-11-08 00:00:00 | +-------------------------------+ Before the fix: +-------------------------------+ | CAST(20061108.01 AS DATETIME) | +-------------------------------+ | 0000-00-20 06:11:08 | +-------------------------------+ After the fix: +-------------------------------+ | cast(20061108.01 as datetime) | +-------------------------------+ | 2006-11-08 00:00:00 | +-------------------------------+
Warnings: -Warning 1292 Incorrect datetime value: '20061108.01' +Warning 1292 Truncated incorrect datetime value: '20061108.01' End of 4.1 tests select time_format('100:00:00', '%H %k %h %I %l'); time_format('100:00:00', '%H %k %h %I %l') === modified file 'mysql-test/r/old-mode.result' --- mysql-test/r/old-mode.result 2014-03-07 20:05:28 +0000 +++ mysql-test/r/old-mode.result 2014-04-14 12:51:24 +0000 @@ -95,8 +95,9 @@ INSERT INTO t1 VALUES (NULL, '00:20:12') INSERT INTO t1 VALUES (NULL, '-00:20:12'); SELECT IF(1,ADDDATE(IFNULL(a,b),0),1) FROM t1; IF(1,ADDDATE(IFNULL(a,b),0),1) -0000-00-00 00:20:12 +NULL NULL Warnings: +Warning 1292 Incorrect datetime value: '0000-00-00 00:20:12'
Why? Because ADDDATE() requires a valid DATETIME value?
Yes. Before the fix Item_func_nullif::get_date() incorrectly returned MYSQL_TIMESTAMP_TIME in this context, so there were no check_date(), which was wrong. After the fix it returns MYSQL_TIMESTAMP_DATETIME, so check_date() is now called, which is correct.
Warning 1292 Truncated incorrect datetime value: '-00:20:12' DROP TABLE t1;
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
It converts 1.0e+10 to datetime "2001-00-00 00:00:00". It looks correct to produce a note instead of a warning here. Would you like it to be reported separately?
Warning 1264 Out of range value for column 'c17' at row 1 insert into t9 ( c1, c13, c14, c15, c16, c17 ) === modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
They should never appear in this context. MYSQL_TYPE_XXX2 can only be returned from field->real_type(), and should never be returned from item->field_type().
+ return true; + default: + return false; + } +} + === modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
Conversion is needed only if args[0] is datetime and arg[1] is TIME, or the other way around. When both args[0] and args[1] are TIMEs, no conversion is needed for performance purposes. So knowing only warn_item->field_type() is not enough. This chunk implements this logic: --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -593,6 +593,11 @@ int Arg_comparator::set_compare_func(Ite switch (type) { case TIME_RESULT: cmp_collation.collation= &my_charset_numeric; + if ((a[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(b[0]->field_type())) || + (b[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(a[0]->field_type()))) + convert_time_to_datetime= true;
{ longlong UNINIT_VAR(value); Item *item= **item_arg;
Regards, Sergei
Hi, Alexander! On May 05, Alexander Barkov wrote:
=== modified file 'mysql-test/r/cast.result' --- mysql-test/r/cast.result 2014-03-26 21:25:38 +0000 +++ mysql-test/r/cast.result 2014-04-09 13:32:17 +0000 @@ -327,7 +327,7 @@ cast(cast(120010203101112.121314 as doub NULL select cast(cast(1.1 as double) as datetime); cast(cast(1.1 as double) as datetime) -0000-00-00 00:00:01 +NULL
Why? I'd expect 2000-00-00 00:00:01, in line with your other changes
select cast(cast(-1.1 as double) as datetime); cast(cast(-1.1 as double) as datetime) NULL
Before the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | 0000-00-00 00:00:01 | 0000-00-00 00:00:01 | +---------------------+-----------------------+-------------------------+ 1 row in set, 1 warning (0.01 sec)
After the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | NULL | NULL | +---------------------+-----------------------+-------------------------+ 1 row in set, 3 warnings (0.00 sec)
The version "after the fix" is more consistent, and is MySQL-5.6 compatible.
Okay, it's the same issue as below. 1 is interpreted as date, YYYYMMDD, 0000-00-01, while 1.1 is read as time, HHMMSS.uuuuuu, 00:00:01.100000 See below.
=== modified file 'mysql-test/r/func_time.result' --- mysql-test/r/func_time.result 2014-03-23 10:22:44 +0000 +++ mysql-test/r/func_time.result 2014-04-15 06:56:53 +0000 @@ -1091,9 +1091,9 @@ NULL select isnull(week(now() + 0)), isnull(week(now() + 0.2)), week(20061108), week(20061108.01), week(20061108085411.000002); isnull(week(now() + 0)) isnull(week(now() + 0.2)) week(20061108) week(20061108.01) week(20061108085411.000002) -0 0 45 NULL 45 +0 0 45 45 45
Why? Here you make 20061108.01 to be casted to datetime as "2006-11-06 ??:??:??" I'm not sure that's a good idea. Old code assumed that a non-zero fractional part *always* means sub-seconds, which automatically means the number should be interpreted as YYYYMMDDHHMMSS.uuuuuu, it cannot be possibly interpreted as YYYYMMDD.uuuuuu
Producing very different results for "CAST(20061108.01 AS DATETIME)" and "CAST(20061108.00 AS DATETIME)" looks confusing for me. I think this is the integer part who should determine how the number converts to date/datetime.
Ignoring the fractional part is confusing too. I'd say that *guessing* what the integer means is always error-prone, it's guessing, after all. So any additional hint that helps to parse the number unambiguosly is, actually, good. We use the logic that a fractional part always means microseconds, and that also allows to interpret the number unambiguosly. I'm not sure this can be changed in a GA version.
Also, the version after fix is MySQL-5.6 compatible:
Yes, but still we don't have to be bug-compatible with 5.6.
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
It converts 1.0e+10 to datetime "2001-00-00 00:00:00". It looks correct to produce a note instead of a warning here.
Would you like it to be reported separately?
At least, committed separately, if possible. I found "bzr shelve" command to be very useful in cases like that.
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
They should never appear in this context.
MYSQL_TYPE_XXX2 can only be returned from field->real_type(), and should never be returned from item->field_type().
Could you add them with an assert then, please? Like case MYSQL_TYPE_DATE2: case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: DBUG_ASSERT(0); // impossible case MYSQL_TYPE_DATE: case MYSQL_TYPE_DATETIME: case MYSQL_TYPE_TIMESTAMP: return true; etc
=== modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
Conversion is needed only if args[0] is datetime and arg[1] is TIME, or the other way around.
When both args[0] and args[1] are TIMEs, no conversion is needed for performance purposes. So knowing only warn_item->field_type() is not enough.
This chunk implements this logic:
--- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -593,6 +593,11 @@ int Arg_comparator::set_compare_func(Ite switch (type) { case TIME_RESULT: cmp_collation.collation= &my_charset_numeric; + if ((a[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(b[0]->field_type())) || + (b[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(a[0]->field_type()))) + convert_time_to_datetime= true;
Yes, that's what I mean. Instead of passing convert_time_to_datetime from the caller, you can put this logic inside get_datetime_value(): if ((item->field_type() == MYSQL_TYPE_TIME && is_temporal_type_with_date(warn_item->field_type())) ... this if() means that TIME value will be compared as datetime, and thus needs to be converted. Regards, Sergei
Hello Sergei, Thanks for review. Please see my replies below: On 05/28/2014 09:17 PM, Sergei Golubchik wrote:
Hi, Alexander!
On May 05, Alexander Barkov wrote:
=== modified file 'mysql-test/r/cast.result' --- mysql-test/r/cast.result 2014-03-26 21:25:38 +0000 +++ mysql-test/r/cast.result 2014-04-09 13:32:17 +0000 @@ -327,7 +327,7 @@ cast(cast(120010203101112.121314 as doub NULL select cast(cast(1.1 as double) as datetime); cast(cast(1.1 as double) as datetime) -0000-00-00 00:00:01 +NULL
Why? I'd expect 2000-00-00 00:00:01, in line with your other changes
select cast(cast(-1.1 as double) as datetime); cast(cast(-1.1 as double) as datetime) NULL
Before the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | 0000-00-00 00:00:01 | 0000-00-00 00:00:01 | +---------------------+-----------------------+-------------------------+ 1 row in set, 1 warning (0.01 sec)
After the fix: +---------------------+-----------------------+-------------------------+ | CAST(1 AS DATETIME) | CAST(1.1 AS DATETIME) | CAST(1.1e0 AS DATETIME) | +---------------------+-----------------------+-------------------------+ | NULL | NULL | NULL | +---------------------+-----------------------+-------------------------+ 1 row in set, 3 warnings (0.00 sec)
The version "after the fix" is more consistent, and is MySQL-5.6 compatible.
Okay, it's the same issue as below. 1 is interpreted as date, YYYYMMDD, 0000-00-01, while 1.1 is read as time, HHMMSS.uuuuuu, 00:00:01.100000
See below.
=== modified file 'mysql-test/r/func_time.result' --- mysql-test/r/func_time.result 2014-03-23 10:22:44 +0000 +++ mysql-test/r/func_time.result 2014-04-15 06:56:53 +0000 @@ -1091,9 +1091,9 @@ NULL select isnull(week(now() + 0)), isnull(week(now() + 0.2)), week(20061108), week(20061108.01), week(20061108085411.000002); isnull(week(now() + 0)) isnull(week(now() + 0.2)) week(20061108) week(20061108.01) week(20061108085411.000002) -0 0 45 NULL 45 +0 0 45 45 45
Why? Here you make 20061108.01 to be casted to datetime as "2006-11-06 ??:??:??" I'm not sure that's a good idea. Old code assumed that a non-zero fractional part *always* means sub-seconds, which automatically means the number should be interpreted as YYYYMMDDHHMMSS.uuuuuu, it cannot be possibly interpreted as YYYYMMDD.uuuuuu
Producing very different results for "CAST(20061108.01 AS DATETIME)" and "CAST(20061108.00 AS DATETIME)" looks confusing for me. I think this is the integer part who should determine how the number converts to date/datetime.
Ignoring the fractional part is confusing too. I'd say that *guessing* what the integer means is always error-prone, it's guessing, after all. So any additional hint that helps to parse the number unambiguosly is, actually, good. We use the logic that a fractional part always means microseconds, and that also allows to interpret the number unambiguosly.
I still think that we should rely only on the range and should not check the fractional part. The current behavior is confusing: +----------------------------+-----------------------------+ | cast(1.000001 as datetime) | cast(1.0000001 as datetime) | +----------------------------+-----------------------------+ | 0000-00-00 00:00:01 | NULL | +----------------------------+-----------------------------+ If the non-zero fractional part fits into 6 digits, it considers the fractional part as microseconds. If the leading 6 fractional digits are zeros and there are some non-zeros in the 7th position, it works differently and obviously breaks the rule "treat as TIME if there is a fractional part". With floating point numbers it gets even more confusing to rely on the fractional part, because "fractional part is zero" is a very non-precise fact. Currently it works confusingly: - For FLOAT: drop table if exists t1; create table t1 (a float(20,10)); insert into t1 values (8001010.1),(1001010.1); select a, cast(a as datetime) from t1; +--------------------+---------------------+ | a | cast(a as datetime) | +--------------------+---------------------+ | 8001010.0000000000 | NULL | | 1001010.1250000000 | 0000-00-01 00:10:10 | +--------------------+---------------------+ - And even for DOUBLE: select cast(101231.000001e0 as datetime),cast(101231.00001e0 as datetime); +-----------------------------------+----------------------------------+ | cast(101231.000001e0 as datetime) | cast(101231.00001e0 as datetime) | +-----------------------------------+----------------------------------+ | 2010-12-31 00:00:00 | 0000-00-00 10:12:31 | +-----------------------------------+----------------------------------+ 1 row in set (30.76 sec) Note sure why a double number 101231.000001 is a worse time than a double number 101231.00001 Notice, the fractional part is non-zero within the 6 digits in both numbers! So it's even more confusing than the above example with DECIMALs.
I'm not sure this can be changed in a GA version.
I think this is a very rare issue and it's unlikely that somebody will really hit this. Having consistency at a very low risk sounds legitimate here for me.
Also, the version after fix is MySQL-5.6 compatible:
Yes, but still we don't have to be bug-compatible with 5.6.
Not sure what you mean :) MySQL behavior is better here, and this is not a bug.
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
It converts 1.0e+10 to datetime "2001-00-00 00:00:00". It looks correct to produce a note instead of a warning here.
Would you like it to be reported separately?
At least, committed separately, if possible. I found "bzr shelve" command to be very useful in cases like that.
Okey.
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
They should never appear in this context.
MYSQL_TYPE_XXX2 can only be returned from field->real_type(), and should never be returned from item->field_type().
Could you add them with an assert then, please? Like
case MYSQL_TYPE_DATE2: case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: DBUG_ASSERT(0); // impossible case MYSQL_TYPE_DATE: case MYSQL_TYPE_DATETIME: case MYSQL_TYPE_TIMESTAMP: return true;
etc
Okey.
=== modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
Conversion is needed only if args[0] is datetime and arg[1] is TIME, or the other way around.
When both args[0] and args[1] are TIMEs, no conversion is needed for performance purposes. So knowing only warn_item->field_type() is not enough.
This chunk implements this logic:
--- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -593,6 +593,11 @@ int Arg_comparator::set_compare_func(Ite switch (type) { case TIME_RESULT: cmp_collation.collation= &my_charset_numeric; + if ((a[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(b[0]->field_type())) || + (b[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(a[0]->field_type()))) + convert_time_to_datetime= true;
Yes, that's what I mean. Instead of passing convert_time_to_datetime from the caller, you can put this logic inside get_datetime_value():
if ((item->field_type() == MYSQL_TYPE_TIME && is_temporal_type_with_date(warn_item->field_type())) ...
this if() means that TIME value will be compared as datetime, and thus needs to be converted.
IIRC, I saw code pieces when the "warn_item" is not really always the second comparison counterpart but something else. Will check this again.
Regards, Sergei
Hi, Alexander! On May 29, Alexander Barkov wrote:
I still think that we should rely only on the range and should not check the fractional part.
...
If the non-zero fractional part fits into 6 digits, it considers the fractional part as microseconds.
If the leading 6 fractional digits are zeros and there are some non-zeros in the 7th position, it works differently and obviously breaks the rule "treat as TIME if there is a fractional part".
With floating point numbers it gets even more confusing to rely on the fractional part, because "fractional part is zero" is a very non-precise fact.
Okay, let's change it :) Regards, Sergei
Hello Sergei, Please find a new version of the patch that addresses your review suggestions. Also, see comments inline. Thanks. On 05/28/2014 09:17 PM, Sergei Golubchik wrote: <skip>
=== modified file 'mysql-test/r/ps_2myisam.result' --- mysql-test/r/ps_2myisam.result 2013-11-20 11:05:39 +0000 +++ mysql-test/r/ps_2myisam.result 2014-04-15 04:25:48 +0000 @@ -3256,7 +3256,7 @@ values ( 50, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10, 1.0e+10 ) ; Warnings: Warning 1265 Data truncated for column 'c15' at row 1 -Warning 1265 Data truncated for column 'c16' at row 1 +Note 1265 Data truncated for column 'c16' at row 1
What bug number is this for? Doesn't look like any of those five that you've listed.
It converts 1.0e+10 to datetime "2001-00-00 00:00:00". It looks correct to produce a note instead of a warning here.
Would you like it to be reported separately?
At least, committed separately, if possible. I found "bzr shelve" command to be very useful in cases like that.
I reported and pushed it as a separate bug: MDEV-6287 Bad warning level when inserting a DATETIME value into a TIME column
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-04-11 10:32:12 +0000 @@ -94,6 +94,28 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP:
What about MYSQL_TYPE_DATETIME2, MYSQL_TYPE_TIMESTAMP2 types?
They should never appear in this context.
MYSQL_TYPE_XXX2 can only be returned from field->real_type(), and should never be returned from item->field_type().
Could you add them with an assert then, please? Like
case MYSQL_TYPE_DATE2: case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: DBUG_ASSERT(0); // impossible case MYSQL_TYPE_DATE: case MYSQL_TYPE_DATETIME: case MYSQL_TYPE_TIMESTAMP: return true;
Done.
=== modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -881,7 +886,8 @@ void Arg_comparator::set_datetime_cmp_fu
longlong get_datetime_value(THD *thd, Item ***item_arg, Item **cache_arg, - Item *warn_item, bool *is_null) + Item *warn_item, bool *is_null, + bool convert_time_to_datetime)
do you need this convert_time_to_datetime? It might be enough to use is_temporal_type_with_date(warn_item->field_type())
Conversion is needed only if args[0] is datetime and arg[1] is TIME, or the other way around.
When both args[0] and args[1] are TIMEs, no conversion is needed for performance purposes. So knowing only warn_item->field_type() is not enough.
This chunk implements this logic:
--- sql/item_cmpfunc.cc 2014-03-26 21:25:38 +0000 +++ sql/item_cmpfunc.cc 2014-04-15 02:15:58 +0000 @@ -593,6 +593,11 @@ int Arg_comparator::set_compare_func(Ite switch (type) { case TIME_RESULT: cmp_collation.collation= &my_charset_numeric; + if ((a[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(b[0]->field_type())) || + (b[0]->field_type() == MYSQL_TYPE_TIME && + is_temporal_type_with_date(a[0]->field_type()))) + convert_time_to_datetime= true;
Yes, that's what I mean. Instead of passing convert_time_to_datetime from the caller, you can put this logic inside get_datetime_value():
if ((item->field_type() == MYSQL_TYPE_TIME && is_temporal_type_with_date(warn_item->field_type())) ...
this if() means that TIME value will be compared as datetime, and thus needs to be converted.
Done. Thant worked fine. Thanks.
Regards, Sergei
Hi, Alexander! On Jun 02, Alexander Barkov wrote:
Hello Sergei,
Please find a new version of the patch that addresses your review suggestions. Also, see comments inline.
Thanks.
It looks like you've mistakenly attached the first version of the patch. It's exactly identical to what you've sent earlier. Regards, Sergei
Hi Sergei, On 06/02/2014 06:01 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jun 02, Alexander Barkov wrote:
Hello Sergei,
Please find a new version of the patch that addresses your review suggestions. Also, see comments inline.
Thanks.
It looks like you've mistakenly attached the first version of the patch. It's exactly identical to what you've sent earlier.
Oops. Something wrong happened. Sorry for that. Attached.
Regards, Sergei
Hi, Alexander! Looks ok to push, thanks! One minor comment below. On Jun 02, Alexander Barkov wrote:
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-06-02 10:13:23 +0000 @@ -94,6 +94,31 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + return true; + case MYSQL_TYPE_DATETIME2: + case MYSQL_TYPE_TIMESTAMP2: + DBUG_ASSERT(0); // field->real_type() should not get to here.
In my earlier comment, where I suggested this assert, I've put it before other cases. That is, in optimized builds, when asserts are disabled, is_temporal_type_with_date() would still return true for MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2. You've put it too late, it would return false for these types. Admittedly, it's not terribly important, so fix it or not, as you like.
+ default: + return false; + } +}
Regards, Sergei
Hi Sergei, On 06/02/2014 07:10 PM, Sergei Golubchik wrote:
Hi, Alexander!
Looks ok to push, thanks!
Thanks for review!
One minor comment below.
an answer to your comment below:
On Jun 02, Alexander Barkov wrote:
=== modified file 'sql/field.h' --- sql/field.h 2014-03-26 21:25:38 +0000 +++ sql/field.h 2014-06-02 10:13:23 +0000 @@ -94,6 +94,31 @@ inline uint get_set_pack_length(int elem
/** + Tests if field type is temporal and has date part, + i.e. represents DATE, DATETIME or TIMESTAMP types in SQL. + + @param type Field type, as returned by field->type(). + @retval true If field type is temporal type with date part. + @retval false If field type is not temporal type with date part. +*/ +inline bool is_temporal_type_with_date(enum_field_types type) +{ + switch (type) + { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + return true; + case MYSQL_TYPE_DATETIME2: + case MYSQL_TYPE_TIMESTAMP2: + DBUG_ASSERT(0); // field->real_type() should not get to here.
In my earlier comment, where I suggested this assert, I've put it before other cases. That is, in optimized builds, when asserts are disabled, is_temporal_type_with_date() would still return true for MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2.
You've put it too late, it would return false for these types.
Admittedly, it's not terribly important, so fix it or not, as you like.
The MYSQL_TYPE_XXX2 types should not appear here. So returning false is Ok. And it can be faster, as in a release build the code is equal to: ... case MYSQL_TYPE_DATETIME2: case MYSQL_TYPE_TIMESTAMP2: default: return false; so a smart compiler should just remove any tests for MYSQL_TYPE_DATETIME2 and MYSQL_TYPE_TIMESTAMP2. (I didn't check though if gcc really removes the tests)
+ default: + return false; + } +}
Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik