maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Mar 06, Alexander Barkov wrote:
> >> +/* 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)
> 
> I replaced more instances of 86400 right now, so
> no separate change is needed any more.

I remember when I grepped, there was one instance on 86400000
(microseconds, I guess). That's why I wrote above "86400" and not
"86400L" :)

> This constant already exists in MySQL-5.6, so I just reused the name.

Okay, that's a good reason.

> >> === 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
> >> === 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?
> 
> This is the most difficult part :)
> Can you suggest please?
> 
> get_temporal_not_time() does not sound well.
> 
> > Why did you need a new function, shouldn't get_date() be doing it?
> 
> get_date() is not enough any more.
> 
> get_date() returns in "native" format by default,
> and I don't change this, because some temporal hybrid functions
> like ADDTIME use this:
...
> EXTRACT also wants the native format.

Yes, but we already have flags for that.

TIME_DATETIME_ONLY only means that you need to convert to datetime.
If there is no TIME_DATETIME_ONLY, then you return the native format.

It's only

  if (fuzzydate & TIME_DATETIME_ONLY &&
      ltime->type == MYSQL_TIMESTAMP_TIME)
    {
      MYSQL_TIME tmp;
      time_to_datetime(current_thd, ltime, &tmp);
      *ltime= tmp;
    }

On the other hand... get_date() is virtual, so we'd need this code in
every get_date() implementation... So, perhaps non-virtual wrapper would
be good, okay.

A name could be get_date_with_conversion(). We already have
set_field_to_null_with_conversions, fetch_datetime_with_conversion, etc.

> >> === 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?
> 
> I added excerpts into:
> https://mariadb.atlassian.net/browse/MDEV-5372

Right, it describes the conversion between TIME and DATETIME, where both
are, as the standard calls them, "datetime types". See 4.6.2 Datetimes.

It also says that

  The <primary datetime field>s other than SECOND contain non-negative
  integer values, constrained by the natural rules for dates using the
  Gregorian calendar. SECOND, however, can be defined to have a <time
  fractional seconds precision>

Here <primary datetime field> is one of YEAR, MONTH, DAY, HOUR, MINUTE,
SECOND.

Anyway, this means that a TIME that can be casted to a DATETIME must be
between 00:00:00 and 23:59:59.999999

Our TIME type is more like an interval type from the standard, and
acording to the standard one cannot cast an interval to a datetime, it's
not allowed.

Anyway, I think your implementation is ok (I just wanted to know whether
it's the standard behavior or an extension), but asserts might be not.
What if someone does SET TIMESTAMP='0000-00-00 00:00:01' ?

> >> === 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)
> 
> Probably these are bad function names.
> They are not actually restricted to the source type.
> Should I rename the functions to just:
> 
> to_time()
> to_date()
> to_datetime()

Not really, they're exactly datetime_to_date() and date_to_datetime().
See, I didn't comment on time_to_datetime(), I didn't say you need an
assert there. Because any time could be converted to
MYSQL_TIMESTAMP_TIME with that function.

But datetime_to_date() simply sets the type, so if you'd pass
MYSQL_TIMESTAMP_TIME as input, the result will be 0000-00-00,
not CURRENT_DATE(). I know that it's a low-level function that shouldn't
use current_thd, but it would be confusing to have two ways of casting
TIME to DATE. Better to limit datetime_to_date() to DATETIME type only.
I mean, "limit" (in quotes), because it actually doesn't limit anything,
you don't use it for MYSQL_TIMESTAMP_TIME anyway.

> === modified file 'sql/item.cc'
> --- sql/item.cc	2014-02-28 09:00:31 +0000
> +++ sql/item.cc	2014-03-06 10:22:00 +0000
> @@ -235,6 +235,50 @@ bool Item::val_bool()
>  
>  
>  /*
> +  Get date/time/datetime.
> +  Optionally extend TIME result to DATETIME.
> +*/
> +bool Item::get_date_date(MYSQL_TIME *ltime, ulonglong fuzzydate)
> +{
> +  /*
> +    Some TIME type items return error when trying to do get_date()
> +    without TIME_TIME_ONLY set (e.g. Item_field for Field_time).
> +    In the SQL standard time->datetime conversion mode we add TIME_TIME_ONLY.
> +    In the legacy time->datetime conversion mode we let the Item to check date,
> +    like it was before.
> +  */
> +  ulonglong time_flag= (field_type() == MYSQL_TYPE_TIME &&
> +                       !current_thd->variables.old_mode) ? TIME_TIME_ONLY : 0;

please, only do one current_thd per function

> +  if (get_date(ltime, fuzzydate | time_flag))
> +    return true;
> +  if (ltime->time_type == MYSQL_TIMESTAMP_TIME &&
> +      !(fuzzydate & TIME_TIME_ONLY))
> +  {
> +    MYSQL_TIME tmp;
> +    THD *thd= current_thd;
> +    int warn= 0;
> +    /*
> +      After time_to_datetime() we need to do check_date(), as
> +      the caller may want TIME_NO_ZERO_DATE or TIME_NO_ZERO_IN_DATE.
> +      Note, the SQL standard time->datetime conversion mode always returns
> +      a valid date based on CURRENT_DATE. So we need to do check_date()
> +      only in the old mode.
> +    */
> +    if (time_to_datetime(thd, ltime, &tmp) ||
> +        (thd->variables.old_mode && check_date(&tmp, fuzzydate, &warn)))
> +    {
> +      ErrConvTime str(ltime);
> +      make_truncated_value_warning(thd, Sql_condition::WARN_LEVEL_WARN,
> +                                   &str, MYSQL_TIMESTAMP_DATETIME, 0); 
> +      return true;
> +    }
> +    *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.

Regards,
Sergei