Hi Satya,
after some speculations about the effect of the patch, i came to the
conclusion that it is probably correct.
However, i cannot approve it anyway. I am missing comments. Please see
below.
Satya B, 04.11.2008 06:24:
..
> 2900 Satya B 2008-11-04
> Fix for Bug#33696 - CSV storage engine allows nullable columns via ALTER TABLE
> statements
Good that you have bug number and synopsis.
In addition, we need as the revision comment:
1. What was the problem. The reader should not need to look up the bug
report to figure out, what problem has been solved. The same time saver
applies to the docs team, who have to add this information to the changelog.
2. How was it solved. Not everyone can immediately see from the code
changes, how the patch works. It took me some time to figure out, how
the stuff around check_if_incompatible_data() works. Even though I had
seen it before. But there were major changes in this area.
So please always have these two sections in bug fixes.
> added:
> mysql-test/r/csv_alter_table.result
I was surprised that you did not include csv_alter_table.test in your
patch. But then I figured out that it pre-existed and was disabled.
Since this is an exceptional case, please add a sentence to the revision
comment above, so that others don't become as confused as I.
> modified:
...
> === modified file 'storage/csv/ha_tina.cc'
> --- a/storage/csv/ha_tina.cc 2008-08-23 00:18:35 +0000
> +++ b/storage/csv/ha_tina.cc 2008-11-04 05:24:38 +0000
> @@ -1598,7 +1598,10 @@ int ha_tina::check(THD* thd, HA_CHECK_OP
> bool ha_tina::check_if_incompatible_data(HA_CREATE_INFO *info,
> uint table_changes)
> {
> - return COMPATIBLE_DATA_YES;
> + if (table_changes == IS_EQUAL_NO)
> + return COMPATIBLE_DATA_NO;
> + else
> + return COMPATIBLE_DATA_YES;
> }
I guess, you wanted to keep the implementation of
ha_tina::check_if_incompatible_data() compatible to the other engines:
No comment, how it works. ;-)
There's not even a function comment. Since you kept the common style, I
can't really complain. But adding comments to code, explaining what it
does, is something that could make you stand out from the crowd. ;-)
I'll set the bug back to "in progress" until you commit a new patch with
at least the required comments.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
