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