maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Mar 04, Alexander Barkov wrote:
> Hi Sergei,
> 
> (my mailer failed during the previous attempt. writing again,
> sorry if you get this twice)

No problem. I did get it twice, but only the second copy was cc: to the
mailing list, that's why I'm replying to it.

> Please review the patch implementing the standard (and MySQL-5.6)
> compatible way of time to datetime conversion.

Here, I have only very few suggestions, but many questions :) see below

> === modified file 'include/my_time.h'
> --- include/my_time.h	2014-02-19 10:05:15 +0000
> +++ include/my_time.h	2014-03-04 13:04:26 +0000
> @@ -63,6 +63,7 @@ extern uchar days_in_month[];
>  #define TIME_FUZZY_DATES        1
>  #define TIME_DATETIME_ONLY      2
>  #define TIME_TIME_ONLY          4
> +#define TIME_TIME_POSSIBLY      8

Why do you need "possibly"?
One could think that if neither TIME_DATETIME_ONLY
nor TIME_TIME_ONLY is set, then it is "possibly" either one.

>  #define TIME_NO_ZERO_IN_DATE    (1UL << 23) /* == MODE_NO_ZERO_IN_DATE */
>  #define TIME_NO_ZERO_DATE	(1UL << 24) /* == MODE_NO_ZERO_DATE    */
>  #define TIME_INVALID_DATES	(1UL << 25) /* == MODE_INVALID_DATES   */
> @@ -77,6 +78,9 @@ extern uchar days_in_month[];
>  #define MYSQL_TIME_WARN_HAVE_WARNINGS(x) MY_TEST((x) & MYSQL_TIME_WARN_WARNINGS)
>  #define MYSQL_TIME_WARN_HAVE_NOTES(x) MY_TEST((x) & MYSQL_TIME_WARN_NOTES)
>  
> +/* Usefull constants */
> +#define SECONDS_IN_24H 86400L

in a separate follow-up changeset, please replace all instances of 86400
with SECONDS_IN_24H (althought I'd probably use SECONDS_IN_DAY)

> +
>  /* Limits for the TIME data type */
>  #define TIME_MAX_HOUR 838
>  #define TIME_MAX_MINUTE 59
> 
> === modified file 'mysql-test/r/func_time.result'
> --- mysql-test/r/func_time.result	2014-02-04 09:49:44 +0000
> +++ mysql-test/r/func_time.result	2014-03-04 13:13:27 +0000
> @@ -2361,7 +2368,7 @@ HOUR(TIME('1 02:00:00'))	HOUR(TIME('26:0
>  26	26
>  SELECT DAY(TIME('1 02:00:00')), DAY(TIME('26:00:00'));
>  DAY(TIME('1 02:00:00'))	DAY(TIME('26:00:00'))
> -0	0
> +4	4

because 'DAY' is simply another name for 'DAYOFMONTH'
and now different from EXTRACT(DAY ...)
I see.

>  SELECT EXTRACT(HOUR FROM '1 02:00:00'), EXTRACT(HOUR FROM '26:00:00');
>  EXTRACT(HOUR FROM '1 02:00:00')	EXTRACT(HOUR FROM '26:00:00')
>  2	2
> === modified file 'sql-common/my_time.c'
> --- sql-common/my_time.c	2013-12-16 12:02:21 +0000
> +++ sql-common/my_time.c	2014-03-04 13:05:54 +0000
> @@ -81,6 +81,9 @@ uint calc_days_in_year(uint year)
>  my_bool check_date(const MYSQL_TIME *ltime, my_bool not_zero_date,
>                     ulonglong flags, int *was_cut)
>  {
> +  if ((flags & TIME_TIME_POSSIBLY) &&
> +      ltime->time_type == MYSQL_TIMESTAMP_TIME)
> +   return FALSE;

okay, so you don't trust ltime->time_type anymore? why?

>    if (not_zero_date)
>    {
>      if (((flags & TIME_NO_ZERO_IN_DATE) &&
> 
> === modified file 'sql/item.cc'
> --- sql/item.cc	2014-02-28 09:00:31 +0000
> +++ sql/item.cc	2014-03-04 13:27:37 +0000
> @@ -235,6 +235,25 @@ bool Item::val_bool()
>  
>  
>  /*
> +  Get date/time/datetime.
> +  Optionally extended TIME result to DATETIME.
> +*/
> +bool Item::get_date_date(MYSQL_TIME *ltime, ulonglong fuzzydate)

I don't like get_date_date name, can you find a better one, please?
Why did you need a new function, shouldn't get_date() be doing it?

> +{
> +  if (get_date(ltime, fuzzydate | TIME_TIME_POSSIBLY))
> +    return true;
> +  if (ltime->time_type == MYSQL_TIMESTAMP_TIME &&
> +      !(fuzzydate & TIME_TIME_ONLY))
> +  {
> +    MYSQL_TIME tmp;
> +    time_to_datetime(current_thd, ltime, &tmp);
> +    *ltime= tmp;
> +  }
> +  return false;
> +}
> +
> +
> +/*
>    For the items which don't have its own fast val_str_ascii()
>    implementation we provide a generic slower version,
>    which converts from the Item character set to ASCII.
> === modified file 'sql/item_timefunc.cc'
> --- sql/item_timefunc.cc	2014-02-19 10:05:15 +0000
> +++ sql/item_timefunc.cc	2014-03-04 12:30:59 +0000
> @@ -780,7 +780,7 @@ longlong Item_func_to_seconds::val_int_e
>    longlong seconds;
>    longlong days;
>    int dummy;                                /* unused */
> -  if (get_arg0_date(&ltime, TIME_FUZZY_DATES))
> +  if (get_arg0_date(&ltime, TIME_FUZZY_DATES | TIME_INVALID_DATES))

invalid dates? for TO_SECONDS?

>    {
>      /* got NULL, leave the incl_endp intact */
>      return LONGLONG_MIN;
> @@ -858,7 +858,7 @@ longlong Item_func_to_days::val_int_endp
>    MYSQL_TIME ltime;
>    longlong res;
>    int dummy;                                /* unused */
> -  if (get_arg0_date(&ltime, 0))
> +  if (get_arg0_date(&ltime, TIME_FUZZY_DATES | TIME_INVALID_DATES))

invalid dates? for TO_DAYS?

>    {
>      /* got NULL, leave the incl_endp intact */
>      return LONGLONG_MIN;
> @@ -2512,26 +2512,7 @@ bool Item_datetime_typecast::get_date(MY
>    if (decimals < TIME_SECOND_PART_DIGITS)
>      my_time_trunc(ltime, decimals);
>  
> -  /*
> -    ltime is valid MYSQL_TYPE_TIME (according to fuzzy_date).
> -    But not every valid TIME value is a valid DATETIME value!
> -  */
> -  if (ltime->time_type == MYSQL_TIMESTAMP_TIME)
> -  {
> -    if (ltime->neg)
> -    {
> -      ErrConvTime str(ltime);
> -      make_truncated_value_warning(current_thd, Sql_condition::WARN_LEVEL_WARN,
> -                                   &str, MYSQL_TIMESTAMP_DATETIME, 0);
> -      return (null_value= 1);
> -    }
> -    
> -    uint day= ltime->hour/24;
> -    ltime->hour %= 24;
> -    ltime->month= day / 31;
> -    ltime->day= day % 31;
> -  }
> -
> +  DBUG_ASSERT(ltime->time_type != MYSQL_TIMESTAMP_TIME);

why?

>    ltime->time_type= MYSQL_TIMESTAMP_DATETIME;
>    return 0;
>  }
> === modified file 'sql/sql_time.cc'
> --- sql/sql_time.cc	2014-02-19 10:05:15 +0000
> +++ sql/sql_time.cc	2014-03-04 13:26:07 +0000
> @@ -1133,3 +1133,88 @@ void time_to_daytime_interval(MYSQL_TIME
>    ltime->hour%= 24;
>    ltime->time_type= MYSQL_TIMESTAMP_NONE;
>  }
> +
> +
> +/*** Conversion from TIME to DATETIME ***/
> +
> +/*
> +  Simple case: TIME is within normal 24 hours internal.
> +  Mix DATE part of ldate and TIME part of ltime together.
> +*/
> +static void
> +mix_date_and_time_simple(MYSQL_TIME *ldate, const MYSQL_TIME *ltime)
> +{
> +  DBUG_ASSERT(ldate->time_type == MYSQL_TIMESTAMP_DATE ||
> +              ldate->time_type == MYSQL_TIMESTAMP_DATETIME);
> +  ldate->hour= ltime->hour;
> +  ldate->minute= ltime->minute;
> +  ldate->second= ltime->second;
> +  ldate->second_part= ltime->second_part;
> +  ldate->time_type= MYSQL_TIMESTAMP_DATETIME;
> +}
> +
> +
> +/*
> +  Complex case: TIME is negative or outside of the 24 hour interval.
> +*/
> +static void
> +mix_date_and_time_complex(MYSQL_TIME *ldate, const MYSQL_TIME *ltime)
> +{
> +  DBUG_ASSERT(ldate->time_type == MYSQL_TIMESTAMP_DATE ||
> +              ldate->time_type == MYSQL_TIMESTAMP_DATETIME);
> +  longlong seconds;
> +  long days, useconds;
> +  int sign= ltime->neg ? 1 : -1;
> +  ldate->neg= calc_time_diff(ldate, ltime, sign, &seconds, &useconds);
> +
> +  DBUG_ASSERT(!ldate->neg);
> +  DBUG_ASSERT(ldate->year > 0);
> +
> +  days= (long) (seconds / SECONDS_IN_24H);
> +  calc_time_from_sec(ldate, seconds % SECONDS_IN_24H, useconds);
> +  get_date_from_daynr(days, &ldate->year, &ldate->month, &ldate->day);
> +  ldate->time_type= MYSQL_TIMESTAMP_DATETIME;
> +}

what does sql standard says about it?
where does it say, btw, about using the current date at all?

> +
> +
> +/**
> +  Mix a date value and a time value.
> +
> +  @param  IN/OUT  ldate  Date value.
> +  @param          ltime  Time value.
> +*/
> +static void
> +mix_date_and_time(MYSQL_TIME *to, const MYSQL_TIME *from)
> +{
> +  if (!from->neg && from->hour < 24)
> +    mix_date_and_time_simple(to, from);
> +  else
> +    mix_date_and_time_complex(to, from);
> +}
> +
> +
> +/**
> +  Get current date in DATE format
> +*/
> +static void
> +set_current_date(THD *thd, MYSQL_TIME *to)
> +{
> +  thd->variables.time_zone->gmt_sec_to_TIME(to, thd->query_start());
> +  thd->time_zone_used= 1;
> +  datetime_to_date(to);
> +}
> +
> +
> +/**
> +  Convert time to datetime.
> +
> +  The time value is added to the current datetime value.
> +  @param  IN  ltime    Time value to convert from.
> +  @param  OUT ltime2   Datetime value to convert to.
> +*/
> +void
> +time_to_datetime(THD *thd, const MYSQL_TIME *from, MYSQL_TIME *to)
> +{
> +  set_current_date(thd, to);
> +  mix_date_and_time(to, from);
> +}
> 
> === modified file 'sql/sql_time.h'
> --- sql/sql_time.h	2014-02-03 14:22:39 +0000
> +++ sql/sql_time.h	2014-03-04 03:17:57 +0000
> @@ -49,6 +49,21 @@ bool int_to_datetime_with_warn(longlong
>                                 ulonglong fuzzydate,
>                                 const char *name);
>  
> +void time_to_datetime(THD *thd, const MYSQL_TIME *tm, MYSQL_TIME *dt);
> +inline void datetime_to_time(MYSQL_TIME *ltime)
> +{
> +  ltime->year= ltime->month= ltime->day= 0;
> +  ltime->time_type= MYSQL_TIMESTAMP_TIME;
> +}
> +inline void datetime_to_date(MYSQL_TIME *ltime)
> +{

here and below. I'd added asserts for incoming ltime->time_type
like DBUG_ASSERT(ltime->time_type == MYSQL_TIMESTAMP_DATETIME)

> +  ltime->hour= ltime->minute= ltime->second= ltime->second_part= 0;
> +  ltime->time_type= MYSQL_TIMESTAMP_DATE;
> +}
> +inline void date_to_datetime(MYSQL_TIME *ltime)
> +{

DBUG_ASSERT(ltime->time_type == MYSQL_TIMESTAMP_DATE)

> +  ltime->time_type= MYSQL_TIMESTAMP_DATETIME;
> +}
>  void make_truncated_value_warning(THD *thd,
>                                    Sql_condition::enum_warning_level level,
>                                    const ErrConv *str_val,

Regards,
Sergei