Hallo Georgi,
Thank you for your review.
>> 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.
1. Rephrased.
2. Difficult, if we reduce the change to one line as per below. :)
>> +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.
Done.
> 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 ?
All of *mine*, anyway, I think. I have however added a few extra for
the code written by others for your amusement.
>> 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.
A big part of the bug was essentially that you could only have
a call, a type-conversion, possibly a warning, AND a static
error-condition all in one big package, all-or-nothing. The above
interface allowed the original behaviour, the intermediate behaviour
I had that was also internally consistent but introduced too much
behaviourial change for my taste, and the version you saw. Future-
proof. Especially since all the callers are local, so the types are
known at no extra effort, and in the context of that call, the
equivalent of clr.l -(SP); clr.l -(SP); beq.s; beq.s; doesn't really
make much of a difference, in terms of cycles. But whatever.
>> @@ -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.
I've been considering it actually, we could easily do something
along the lines of
if (no_date)
{
*is_null= TRUE;
return -1;
}
here, but I read "5.2.6 - SQL modes" in the manual to suggest
otherwise (throw warning, use near value). It would affect affect
a small class of cases, namely
select str_to_date('2007-10-00 12:34','%Y-%m-%d %H:%i') = ...
(0-day when in a SQL-mode that disallows this), and
select str_to_date('2007-10-00','%Y-%m-%d')
between '' and '2007/10/20';
(empty date string, which would now differ from '0000-00-00', while
of course select str_to_date('','%Y-%m-%d') would still give
'0000-00-00'). Since this does not make handling more uniform (not
just with regard to str_to_date(), of course, < > will also differ
here) and actually seems to contradict the manual, I suggest against
it. In fact, it would essentially reduce to
select str_to_date('2007-10-00','%Y-%m-%d')
between NULL and '2007/10/20';
which in turn renders 0, not NULL, because of the way BETWEEN is
handled (in general, not just with regard to dates) which I'm not
really convinced will make things more intuitive?
On a more abstract level, this would mean that we make one case
more lenient (the reported bug), while adding new restrictions
to another case, which might result in shifting the users'
confusion. IOW, behaviour would still be inconsistent, just in
a new way. Please advise.
thanks/regards,
Tatiana
--
Tatiana A. Nurnberg, Bugs Bunny * MySQL AB, http://www.mysql.de/ * EST
IRC: Azundris * Skype: azundris * MySQL-SIP: 4550 * eMail: azundris@stripped
.de: Firmensitz: MySQL GmbH, Radlkoferstr. 2, D-81373 Muenchen
Geschaeftsfuehrer: Hans von Bell, Kaj Arnoe - HRB Muenchen 162140
