Tatjana,
I want to thank you for the very good analysis of this bug. I have few
ideas/questions about the fix however : see below.
On 02.11.2007, at 11:23, Tatjana A Nuernberg wrote:
> ChangeSet@stripped, 2007-11-02 10:23:25+01:00, tnurnberg@stripped +3 -0
> Bug#31800: Date comparison fails with timezone and slashes for
> greater than comparison
>
> On DATETIME-like literals with trailing garbage, BETWEEN fudged in a
> DATETIME comparator, while greater/less-than used bin-string
> comparisons.
> Now both are compared as DATE or DATETIME (but throw a warning).
Please write somewhat more detailed comment :
1. What exactly was wrong (and mention other cases only for comparison
and identify them explicitly as being correct). I feel that one
statement is not enough here.
2. How exactly have you fixed it. I have the feeling that you
explanation above is too high level. You don't need to explain it at
code level of course, but you need to provide enough detail.
> diff -Nrup a/mysql-test/t/select.test b/mysql-test/t/select.test
> --- a/mysql-test/t/select.test 2007-10-25 07:32:35 +02:00
> +++ b/mysql-test/t/select.test 2007-11-02 10:23:23 +01:00
<snip>
> +#
> +# Bug#31800: Date comparison fails with timezone and slashes for
> greater
> +# than comparison
> +#
> +
> +# On DATETIME-like literals with trailing garbage, BETWEEN fudged
> in a
> +# DATETIME comparator, while greater/less-than used bin-string
> comparisons.
> +# Should correctly be compared as DATE or DATETIME, but throw a
> warning:
> +
> +select str_to_date('2007-10-09','%Y-%m-%d') between '2007/10/01
> 00:00:00 GMT-6' and '2007/10/20 00:00:00 GMT-6' between_check,
> str_to_date('2007-10-09','%Y-%m-%d') > '2007/10/01 00:00:00 GMT-6'
> ge_check, str_to_date('2007-10-09','%Y-%m-%d') <=
> '2007/10/2000:00:00 GMT-6' le_check;
Am I the only one lost in these long lines ? can you please split that
into separate selects (makes them more comprehensible as well). I know
it's the reporter's statement, but we're free to format it so it's
easier to grasp.
> +
> +# We have all we need -- and trailing garbage:
> +select str_to_date('2007-10-01','%Y-%m-%d') = '2007-10-01 00:00:00
> GMT-6';
> +select str_to_date('2007-10-01','%Y-%m-%d') = '2007-10-01 x00:00:00
> GMT-6';
> +select str_to_date('2007-10-01','%Y-%m-%d %H:%i:%s') = '2007-10-01
> 00:00:00 GMT-6';
> +select str_to_date('2007-10-01','%Y-%m-%d %H:%i:%s') = '2007-10-01
> 00:x00:00 GMT-6';
> +# no time at all:
> +select str_to_date('2007-10-01','%Y-%m-%d %H:%i:%s') = '2007-10-01
> x12:34:56 GMT-6';
> +# partial time:
> +select str_to_date('2007-10-01 12:34:00','%Y-%m-%d %H:%i:%s') =
> '2007-10-01 12:34x:56 GMT-6';
> +# fail, different second part:
> +select str_to_date('2007-10-01 12:34:56','%Y-%m-%d %H:%i:%s') =
> '2007-10-01 12:34x:56 GMT-6';
> +# correct syntax, no trailing nonsense -- this one must throw no
> warning:
> +select str_to_date('2007-10-01 12:34:56','%Y-%m-%d %H:%i:%s') =
> '2007-10-01 12:34:56';
> +# no warning, but failure (different hour parts):
> +select str_to_date('2007-10-01','%Y-%m-%d') = '2007-10-01 12:00:00';
> +# succeed:
> +select str_to_date('2007-10-01 12','%Y-%m-%d %H') = '2007-10-01
> 12:00:00';
> +# succeed, but warn for "trailing garbage" (":34"):
> +select str_to_date('2007-10-01 12:34','%Y-%m-%d %H') = '2007-10-01
> 12:00:00';
lot of test cases : good ! But since this is a production release and
you're touching a very low level function I'd check the coverage of my
changes : are all lines executed by the tests ?
> --echo End of 5.0 tests
> diff -Nrup a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> --- a/sql/item_cmpfunc.cc 2007-10-13 08:12:12 +02:00
> +++ b/sql/item_cmpfunc.cc 2007-11-02 10:23:23 +01:00
> @@ -552,56 +552,60 @@ int Arg_comparator::set_compare_func(Ite
> }
>
>
> -/*
> - Convert date provided in a string to the int representation.
> -
> - SYNOPSIS
> - get_date_from_str()
> - thd Thread handle
> - str a string to convert
> - warn_type type of the timestamp for issuing the warning
> - warn_name field name for issuing the warning
> - error_arg [out] TRUE if string isn't a DATETIME or clipping
> occur
> +/**
> + @brief Convert date provided in a string to the int representation.
>
> - DESCRIPTION
> - Convert date provided in the string str to the int
> representation.
> - if the string contains wrong date or doesn't contain it at all
> - then the warning is issued and TRUE returned in the error_arg
> argument.
> - The warn_type and the warn_name arguments are used as the name
> and the
> - type of the field when issuing the warning.
> + @param[in] thd thread handle
> + @param[in] str a string to convert
> + @param[in] warn_type type of the timestamp for issuing the
> warning
> + @param[in] warn_name field name for issuing the warning
> + @param[out] was_cut != 0 if clipping did occur, cf.
> str_to_datetime()
> + pass in NULL for "don't care"
> + @param[out] was_type the type we managed to extract
> + pass in NULL for "don't care"
> +
> + @details Convert date provided in the string str to the int
> + representation. If the string contains wrong date or doesn't
> + contain it at all then a warning is issued. The warn_type and
> + the warn_name arguments are used as the name and the type of the
> + field when issuing the warning. If any input was discarded
> + (trailing or non-timestampy characters), was_cut will be non-
> zero.
> + was_type will return the type str_to_datetime() could correctly
> + extract.
>
> - RETURN
> - converted value.
> + @return
> + converted value. 0 on failure.
> */
>
> static ulonglong
> get_date_from_str(THD *thd, String *str, timestamp_type warn_type,
> - char *warn_name, bool *error_arg)
> + char *warn_name, int *was_cut, timestamp_type
> *was_type)
I really don't think you need the two additional arguments.
Correct me if I'm wrong, but you're not actually using the value
returned in the was_cut argument anywhere. And you use the was_type
argument to do a boolean comparison. And this makes it equal to
returning a boolean.
> {
> ulonglong value= 0;
> - int error;
> + int trunc;
> MYSQL_TIME l_time;
> enum_mysql_timestamp_type ret;
> - *error_arg= TRUE;
>
> ret= str_to_datetime(str->ptr(), str->length(), &l_time,
> (TIME_FUZZY_DATE | MODE_INVALID_DATES |
> (thd->variables.sql_mode &
> (MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE))),
> - &error);
> - if ((ret == MYSQL_TIMESTAMP_DATETIME || ret ==
> MYSQL_TIMESTAMP_DATE))
> - {
> + &trunc);
> + if (was_type)
> + *was_type= ret;
> + if (was_cut)
> + *was_cut= trunc;
> +
> + if (ret == MYSQL_TIMESTAMP_DATETIME || ret == MYSQL_TIMESTAMP_DATE)
> value= TIME_to_ulonglong_datetime(&l_time);
> - *error_arg= FALSE;
> - }
> + else
> + trunc= 1;
>
> - if (error || *error_arg)
> - {
> + if (trunc)
> make_truncated_value_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> str->ptr(), str->length(),
> warn_type, warn_name);
> - *error_arg= TRUE;
This is the real fix right here : still compare as dates even if there
were warnings. Why the rest ?
> - }
> +
> return value;
> }
>
> @@ -677,16 +681,24 @@ Arg_comparator::can_compare_as_dates(Ite
> {
> THD *thd= current_thd;
> ulonglong value;
> - bool error;
> String tmp, *str_val= 0;
> timestamp_type t_type= (date_arg->field_type() ==
> MYSQL_TYPE_DATE ?
> MYSQL_TIMESTAMP_DATE :
> MYSQL_TIMESTAMP_DATETIME);
> + timestamp_type was_type;
> + int was_cut;
>
> str_val= str_arg->val_str(&tmp);
> if (str_arg->null_value)
> return CMP_DATE_DFLT;
> - value= get_date_from_str(thd, str_val, t_type, date_arg-
> >name, &error);
> - if (error)
> + value= get_date_from_str(thd, str_val, t_type, date_arg->name,
> + &was_cut, &was_type);
> + /*
> + Fail only if we didn't get a date at all; just was_type !=
> t_type
> + is not good enough as we can still compare a resulting DATE
> to a
> + DATETIME.
> + */
> + if ((was_type != MYSQL_TIMESTAMP_DATETIME) &&
> + (was_type != MYSQL_TIMESTAMP_DATE))
The above changes in this function are not needed imho : see previous
comments.
> return CMP_DATE_DFLT;
> if (const_value)
> *const_value= value;
> @@ -897,11 +909,10 @@ get_datetime_value(THD *thd, Item ***ite
> */
> if (str)
> {
> - bool error;
> enum_field_types f_type= warn_item->field_type();
> timestamp_type t_type= f_type ==
> MYSQL_TYPE_DATE ? MYSQL_TIMESTAMP_DATE :
> MYSQL_TIMESTAMP_DATETIME;
> - value= get_date_from_str(thd, str, t_type, warn_item->name,
> &error);
> + value= get_date_from_str(thd, str, t_type, warn_item->name,
> NULL, NULL);
I'm a little worried about this one : why ignore the error ? Should we
really ? Imho setting to NULL here on non-recoverable errors will be
appropriate.
Best Regards,
Joro
--
Georgi Kodinov, Senior Software Engineer
MySQL AB, Plovdiv, Bulgaria, www.mysql.com
Office: +359 32 634 397 Mobile: +359 887 700 566 Skype: georgekodinov
Are you MySQL certified? www.mysql.com/certification
