Hi,
Thank you for your excellent analysis. Your fix is correct, but it
needs to be communicated/tested better. Please see my comments below.
On 02.10.2007, at 04:50, Tatjana A Nuernberg wrote:
> Below is the list of changes that have just been committed into a
> local
> 5.0 repository of tnurnberg. When tnurnberg does a push these
> changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-10-02 03:50:44+02:00,
> tnurnberg@stripped +3 -0
> Bug #31253: crash comparing datetime to double
>
> conversion to DATETIME would fail on invalid input, but processing
> was not aborted when in WHERE-clause, leading to a crash. patch
> causes a warning on such out of range input instead.
It would be an idea to structure you commit comment as follow :
1. What the problem was
2. How was it fixed
With regards to your current comment I'd also add why it's OK to set
a null_value (section 11.6. in the ref manual stating that incomplete
dates are returned as NULLs.).
> mysql-test/r/type_datetime.result@stripped, 2007-10-02 03:50:40
> +02:00, tnurnberg@stripped +9 -0
> Bug #31253: crash comparing datetime to double
>
> show that convert to datetime in a WHERE-clause will no longer
> crash
> the server on invalid input, but throw a warning instead. show
> that
> correct input is unaffected by this.
Just as an idea : why not add a comment to the .test file detailing
the correct output ?
Makes it easier to read/observe when (e.g.) merging.
I would also not repeat the bug header in the individual file
comments : in bk revtool you'll always get both the commit comment
and the individual file comment.
> mysql-test/t/type_datetime.test@stripped, 2007-10-02 03:50:40+02:00,
> tnurnberg@stripped +9 -0
> Bug #31253: crash comparing datetime to double
>
> show that convert to datetime in a WHERE-clause will no longer
> crash
> the server on invalid input, but throw a warning instead. show
> that
> correct input is unaffected by this.
See above
>
> sql/item.cc@stripped, 2007-10-02 03:50:40+02:00,
> tnurnberg@stripped +2 -0
> Bug #31253: crash comparing datetime to double
>
> mark result NULL to prevent further processing on non-existent
> value.
See above re. bug title.
> diff -Nrup a/mysql-test/r/type_datetime.result b/mysql-test/r/
> type_datetime.result
> --- a/mysql-test/r/type_datetime.result 2007-06-05 22:25:01 +02:00
> +++ b/mysql-test/r/type_datetime.result 2007-10-02 03:50:40 +02:00
> @@ -427,3 +427,12 @@ f1
> Warnings:
> Warning 1292 Incorrect datetime value: '2007010100000' for column
> 'f1' at row 1
> drop table t1;
> +create table `t1` (`f1` time) engine=myisam;
Please take out the "engine=myisam" here : it's not engine specific.
I'd also name the tables t1, t2, etc (unqoted) and the columns a,b,c
etc (unqoted) : makes for consistent reading experience.
> +insert into `t1` set `f1` = '45:44:44';
> +insert into `t1` set `f1` = '15:44:44';
> +select * from t1 where (convert(`f1`,datetime)) != 1;
> +f1
> +15:44:44
> +Warnings:
> +Warning 1292 Truncated incorrect datetime value: '0000-00-00
> 45:44:44'
> +drop table t1;
I see only one test here, but you're changing 2 functions. Please add
a similar case for the other function.
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
