maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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