maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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