Re: bk commit into 5.0 tree (tnurnberg:1.2526) BUG#31253

[Georgi Kodinov]

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: [phone] Mobile: [phone] Skype: georgekodinov

Are you MySQL certified?  www.mysql.com/certification