← Back to team overview

maria-developers team mailing list archive

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