maria-developers team mailing list archive

[Sergei Golubchik <vuvova@xxxxxxxxx>]

Hi, Alexander!

On Jun 26, Alexander Barkov wrote:
> Hi Serg,
> 
> Please review the patch merging MySQL-5.6 temporal literals.

Here is is, below:

> === modified file 'client/mysqlbinlog.cc'
> --- client/mysqlbinlog.cc	2013-04-15 13:09:22 +0000
> +++ client/mysqlbinlog.cc	2013-06-21 13:31:28 +0000
> @@ -1541,13 +1541,14 @@ the mysql command line client.\n\n");
>  
>  static my_time_t convert_str_to_timestamp(const char* str)
>  {
> -  int was_cut;
> +  MYSQL_TIME_STATUS status;
>    MYSQL_TIME l_time;
>    long dummy_my_timezone;
>    uint dummy_in_dst_time_gap;
> +  
>    /* We require a total specification (date AND time) */
> -  if (str_to_datetime(str, (uint) strlen(str), &l_time, 0, &was_cut) !=
> -      MYSQL_TIMESTAMP_DATETIME || was_cut)
> +  if (str_to_datetime(str, (uint) strlen(str), &l_time, 0, &status) ||
> +      l_time.time_type != MYSQL_TIMESTAMP_DATETIME || status.warnings)

Isn't it better to return l_time.time_type from str_to_datetime?
otherwise you'll always need to write like the above

if (str_to_datetime(...) || l_time.time_type != ...)

>    {
>      error("Incorrect date and time argument: %s", str);
>      exit(1);
> === modified file 'include/my_time.h'
> --- include/my_time.h	2013-05-24 15:09:59 +0000
> +++ include/my_time.h	2013-06-25 06:28:08 +0000
> @@ -78,14 +78,27 @@ extern uchar days_in_month[];
>  #define TIME_MAX_VALUE_SECONDS (TIME_MAX_HOUR * 3600L + \
>                                  TIME_MAX_MINUTE * 60L + TIME_MAX_SECOND)
>  
> +/*
> +  Structure to return status from
> +    str_to_datetime(), str_to_time(), number_to_datetime(), number_to_time()
> +*/
> +typedef struct st_mysql_time_status
> +{
> +  int warnings;
> +  uint fractional_digits;

could you please rename it to "precision" ?
or at least, add a comment explaining what it contains

> +} MYSQL_TIME_STATUS;
> +
> +static inline void my_time_status_init(MYSQL_TIME_STATUS *status)
> +{
> +  status->warnings= status->fractional_digits= 0;
> +}
> +
>  my_bool check_date(const MYSQL_TIME *ltime, my_bool not_zero_date,
>                     ulonglong flags, int *was_cut);
> -enum enum_mysql_timestamp_type
> -str_to_time(const char *str, uint length, MYSQL_TIME *l_time, 
> -            ulonglong flag, int *warning);
> -enum enum_mysql_timestamp_type
> -str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time,
> -                ulonglong flags, int *was_cut);
> +my_bool str_to_time(const char *str, uint length, MYSQL_TIME *l_time, 
> +                    ulonglong flag, MYSQL_TIME_STATUS *status);
> +my_bool str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time,
> +                        ulonglong flags, MYSQL_TIME_STATUS *status);
>  longlong number_to_datetime(longlong nr, ulong sec_part, MYSQL_TIME *time_res,
>                              ulonglong flags, int *was_cut);
>  
> === added file 'mysql-test/r/temporal_literal.result'
> --- mysql-test/r/temporal_literal.result	1970-01-01 00:00:00 +0000
> +++ mysql-test/r/temporal_literal.result	2013-06-26 09:12:13 +0000
> @@ -0,0 +1,412 @@
> +DROP TABLE IF EXISTS t1, t2;
> +SET NAMES latin1;
> +#
> +# Testing DATE literals
> +#
> +SELECT DATE'xxxx';
> +ERROR HY000: Incorrect DATE value: 'xxxx'
> +SELECT DATE'01';
> +ERROR HY000: Incorrect DATE value: '01'
> +SELECT DATE'01-01';
> +ERROR HY000: Incorrect DATE value: '01-01'
> +SELECT DATE'2001';
> +ERROR HY000: Incorrect DATE value: '2001'
> +SELECT DATE'2001-01';
> +ERROR HY000: Incorrect DATE value: '2001-01'
> +SELECT DATE'2001-00-00';
> +DATE'2001-00-00'
> +2001-00-00
> +SELECT DATE'2001-01-00';
> +DATE'2001-01-00'
> +2001-01-00
> +SELECT DATE'0000-00-00';
> +DATE'0000-00-00'
> +0000-00-00
> +SELECT DATE'2001-01-01 00:00:00';
> +ERROR HY000: Incorrect DATE value: '2001-01-01 00:00:00'
> +SELECT DATE'01:01:01';
> +DATE'01:01:01'
> +2001-01-01
> +SELECT DATE'01-01-01';
> +DATE'01-01-01'
> +2001-01-01
> +SELECT DATE'2010-01-01';
> +DATE'2010-01-01'
> +2010-01-01
> +SELECT DATE '2010-01-01';
> +DATE '2010-01-01'
> +2010-01-01
> +CREATE TABLE t1 AS SELECT DATE'2010-01-01';
> +SHOW CREATE TABLE t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `DATE'2010-01-01'` date NOT NULL DEFAULT '0000-00-00'
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> +CREATE TABLE t1 AS SELECT
> +{d'2001-01-01'},
> +{d'2001-01-01 10:10:10'};

This (the second value, I mean) is what, backward compatibility?

> +SHOW CREATE TABLE t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `{d'2001-01-01'}` date NOT NULL DEFAULT '0000-00-00',
> +  `2001-01-01 10:10:10` varchar(19) NOT NULL DEFAULT ''
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP TABLE t1;
> +EXPLAIN EXTENDED SELECT {d'2010-01-01'};
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	NULL	NULL	No tables used
> +Warnings:
> +Note	1003	select DATE'2010-01-01' AS `{d'2010-01-01'}`
> +EXPLAIN EXTENDED SELECT DATE'2010-01-01';
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	filtered	Extra
> +1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	NULL	NULL	No tables used
> +Warnings:
> +Note	1003	select DATE'2010-01-01' AS `DATE'2010-01-01'`
...
> === modified file 'sql-common/my_time.c'
> --- sql-common/my_time.c	2013-04-07 12:00:16 +0000
> +++ sql-common/my_time.c	2013-06-26 09:23:23 +0000
> @@ -230,20 +229,43 @@ static uint skip_digits(const char **str
>  
>      The second part may have an optional .###### fraction part.
>  
> -  RETURN VALUES
> +  NOTES
> +   This function should work with a format position vector as long as the
> +   following things holds:
> +   - All date are kept together and all time parts are kept together
> +   - Date and time parts must be separated by blank
> +   - Second fractions must come after second part and be separated
> +     by a '.'.  (The second fractions are optional)
> +   - AM/PM must come after second fractions (or after seconds if no fractions)
> +   - Year must always been specified.
> +   - If time is before date, then we will use datetime format only if
> +     the argument consist of two parts, separated by space.
> +     Otherwise we will assume the argument is a date.
> +   - The hour part must be specified in hour-minute-second order.

I've removed this comment (NOTES) from when I was adding skip_digits()
and other small helpers. Simply because it was mostly wrong.
It's ok to add it back, if you'd like to, but then, please
fix to to reflect the reality (and fix the language too :)

on the other hand, feel free to remove it again, if you want.
the code is pretty much self-documenting now.

> +
> +    status->warnings is set to:
> +    0                            Value OK
> +    MYSQL_TIME_WARN_TRUNCATED    If value was cut during conversion
> +    MYSQL_TIME_WARN_OUT_OF_RANGE check_date(date,flags) considers date invalid
> +
> +    l_time->time_type is set as follows:
>      MYSQL_TIMESTAMP_NONE        String wasn't a timestamp, like
>                                  [DD [HH:[MM:[SS]]]].fraction.
> +                                l_time is not changed.
>      MYSQL_TIMESTAMP_DATE        DATE string (YY MM and DD parts ok)
>      MYSQL_TIMESTAMP_DATETIME    Full timestamp
>      MYSQL_TIMESTAMP_ERROR       Timestamp with wrong values.
>                                  All elements in l_time is set to 0
> +  RETURN VALUES
> +    0 - Ok
> +    1 - Error
>  */
>  
>  #define MAX_DATE_PARTS 8
>  
> -enum enum_mysql_timestamp_type
> +my_bool
>  str_to_datetime(const char *str, uint length, MYSQL_TIME *l_time,
> -                ulonglong flags, int *was_cut)
> +                ulonglong flags, MYSQL_TIME_STATUS *status)
>  {
>    const char *end=str+length, *pos;
>    uint number_of_fields= 0, digits, year_length, not_zero_date;
> @@ -513,12 +538,26 @@ str_to_time(const char *str, uint length
>        if (field_length-- > 0)
>          value= value*10 + (uint) (uchar) (*str - '0');
>      }
> -    if (field_length > 0)
> -      value*= (long) log_10_int[field_length];
> -    else if (field_length < 0)
> -      *warning|= MYSQL_TIME_WARN_TRUNCATED;
> +    if (field_length >= 0)
> +    {
> +      status->fractional_digits= TIME_SECOND_PART_DIGITS - field_length;
> +      if (field_length > 0)
> +        value*= (long) log_10_int[field_length];
> +    }
> +    else
> +    {
> +      /* Scan digits left after microseconds */
> +      status->fractional_digits= 6;
> +      for ( ; str != end && my_isdigit(&my_charset_latin1, *str); str++)

1. old code set MYSQL_TIME_WARN_TRUNCATED, you don't do that anymore
2. there's skip_digits() helper, why not to use it?
3. actually the whole loop can be probaby replaced by get_digits() call

> +      { }
> +    }
>      date[4]= (ulong) value;
>    }
> +  else if ((end - str) == 1 && *str == '.')
> +  {
> +    str++;
> +    date[4]= 0;

is it for, like, "10:10:10." ?

> +  }
>    else
>      date[4]=0;
>      
> @@ -553,7 +592,7 @@ str_to_time(const char *str, uint length
>    if (date[0] > UINT_MAX || date[1] > UINT_MAX ||
>        date[2] > UINT_MAX || date[3] > UINT_MAX ||
>        date[4] > UINT_MAX)
> -    return MYSQL_TIMESTAMP_ERROR;
> +    return TRUE;

you need to set l_time->type= MYSQL_TIMESTAMP_ERROR
(because you no longer return it). Probably better to do it
unconditionally, early in str_to_time().

>    
>    l_time->year=         0;                      /* For protocol::store_time */
>    l_time->month=        0;
> === modified file 'sql/item_cmpfunc.cc'
> --- sql/item_cmpfunc.cc	2013-06-06 15:51:28 +0000
> +++ sql/item_cmpfunc.cc	2013-06-26 08:12:33 +0000
> @@ -721,19 +721,18 @@ bool get_mysql_time_from_str(THD *thd, S
>                               const char *warn_name, MYSQL_TIME *l_time)
>  {
>    bool value;
> -  int error;
> -  enum_mysql_timestamp_type timestamp_type;
> +  MYSQL_TIME_STATUS status;
>    int flags= TIME_FUZZY_DATE | MODE_INVALID_DATES;
>    ErrConvString err(str);
>  
> +  DBUG_ASSERT(warn_type != MYSQL_TIMESTAMP_TIME); // Serg: why the below code?

I don't remember. Currently warn_type can never be MYSQL_TIMESTAMP_TIME here.
This function is only called from stored_field_cmp_to_item() which
is a historical hack and should go away completely. Together with
this get_mysql_time_from_str()

>    if (warn_type == MYSQL_TIMESTAMP_TIME)
>      flags|= TIME_TIME_ONLY;
>  
> -  timestamp_type= 
> -    str_to_datetime(str->charset(), str->ptr(), str->length(),
> -                    l_time, flags, &error);
> -
> -  if (timestamp_type > MYSQL_TIMESTAMP_ERROR)
> +  if (!str_to_datetime(str->charset(), str->ptr(), str->length(),
> +                       l_time, flags, &status) &&
> +      (l_time->time_type == MYSQL_TIMESTAMP_DATETIME || 
> +       l_time->time_type == MYSQL_TIMESTAMP_DATE))
>      /*
>        Do not return yet, we may still want to throw a "trailing garbage"
>        warning.
> === modified file 'sql/item_timefunc.h'
> --- sql/item_timefunc.h	2013-03-13 21:33:52 +0000
> +++ sql/item_timefunc.h	2013-06-24 07:26:29 +0000
> @@ -526,6 +526,136 @@ class Item_timefunc :public Item_tempora
>  };
>  
>  
> +/**
> +  DATE'2010-01-01'
> +*/
> +class Item_date_literal :public Item_datefunc
> +{
> +  MYSQL_TIME cached_time;
> +public:
> +  /**
> +    Constructor for Item_date_literal.
> +    @param ltime  DATE value.
> +  */
> +  Item_date_literal(MYSQL_TIME *ltime) :Item_datefunc()
> +  {
> +    cached_time= *ltime;
> +    fix_length_and_dec();
> +    fixed= 1;
> +  }
> +  const char *func_name() const { return "date_literal"; }
> +  void print(String *str, enum_query_type query_type);
> +  bool get_date(MYSQL_TIME *ltime, ulonglong fuzzy_date);
> +  void fix_length_and_dec()
> +  {
> +    Item_datefunc::fix_length_and_dec();
> +    set_persist_maybe_null(false);
> +  }
> +  bool check_partition_func_processor(uchar *int_arg)
> +  {
> +    return FALSE;
> +  }
> +  bool basic_const_item() const { return true; }
> +  bool const_item() const { return true; }
> +  table_map used_tables() const { return (table_map) 0L; }
> +  void cleanup()
> +  {
> +    // See Item_basic_const::cleanup()
> +    if (orig_name)
> +      name= orig_name;
> +  }
> +  bool eq(const Item *item, bool binary_cmp) const;
> +};

Hmm. I don't really like this hierarchy. It's not very logical to derive
basic const items from Item_func. And you need to overwrite
lots of virtual methods. For every child too, so there's a lot of
code duplication :(

Would it be better to derive these classes from Item_basic_constant?

> +
> +
> +/**
> +  TIME'10:10:10'
> +*/
> +class Item_time_literal :public Item_timefunc
> +{
> +  MYSQL_TIME cached_time;
> === modified file 'sql/sql_yacc.yy'
> --- sql/sql_yacc.yy	2013-06-06 19:32:29 +0000
> +++ sql/sql_yacc.yy	2013-06-20 11:21:37 +0000
> @@ -8741,7 +8742,49 @@ simple_expr:
>                MYSQL_YYABORT;
>            }
>          | '{' ident expr '}'
> -          { $$= $3; }
> +          {
> +            Item_string *item;
> +            $$= NULL;
> +            /*
> +              If "expr" is reasonably short pure ASCII string literal,
> +              try to parse known ODBC style date, time or timestamp literals,
> +              e.g:
> +              SELECT {d'2001-01-01'};
> +              SELECT {t'10:20:30'};
> +              SELECT {ts'2001-01-01 10:20:30'};
> +            */
> +            if ($3->type() == Item::STRING_ITEM &&
> +               (item= (Item_string *) $3) &&
> +                item->collation.repertoire == MY_REPERTOIRE_ASCII &&
> +                item->str_value.length() < MAX_DATE_STRING_REP_LENGTH * 4)
> +            {
> +              enum_field_types type= MYSQL_TYPE_STRING;
> +              ErrConvString str(&item->str_value);

1. it's a confusing usage of ErrConvString. this class is for error messages
2. why do you convert to system_charset_info at all here?

> +              LEX_STRING *ls= &$2;
> +              if (ls->length == 1)
> +              {
> +                if (ls->str[0] == 'd')  /* {d'2001-01-01'} */
> +                  type= MYSQL_TYPE_DATE;
> +                else if (ls->str[0] == 't') /* {t'10:20:30'} */
> +                  type= MYSQL_TYPE_TIME;
> +              }
> +              else if (ls->length == 2) /* {ts'2001-01-01 10:20:30'} */
> +              {
> +                if (ls->str[0] == 't' && ls->str[1] == 's')
> +                  type= MYSQL_TYPE_DATETIME;
> +              }
> +              if (type != MYSQL_TYPE_STRING)
> +              {
> +                const char *ascii= str.ptr();
> +                $$= create_temporal_literal(YYTHD,
> +                                            ascii, strlen(ascii),
> +                                            system_charset_info,
> +                                            type, false);
> +              }
> +            }
> +            if ($$ == NULL)
> +              $$= $3;
> +          }
>          | MATCH ident_list_arg AGAINST '(' bit_expr fulltext_options ')'
>            {
>              $2->push_front($5);
Regards,
Sergei