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