maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #07228
Re: Please review a patch fixing a few MDEV-6001 blockers
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
Follow ups
References
