From: Alexander Barkov
Date: November 19 2010 5:45pm
Subject: Re: bzr commit into mysql-5.5-bugteam branch (bar:3137) Bug#58190
List-Archive: http://lists.mysql.com/commits/124504
Message-Id: <4CE6B7AE.9040500@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Alik,

Thanks for your review.
Please find a new version here:

http://lists.mysql.com/commits/124502

I moved the new method from Field to Field_str,
and numeric types do not need this,
and addressed your suggestions.

Please see comments inline:


Alexander Nozdrin wrote:
> Hello,
> 
> thank you for working on this!
> 
> As discussed on IRC, this quick fix is Ok for 5.5, but please submit
> a new bug for a complete solution.

Done: bug#58329

> 
> Generally, the patch is Ok to push. However, I have a few suggestions
> below.
> 
> On 19.11.2010 17:49, Alexander Barkov wrote:
>>   3137 Alexander Barkov    2010-11-19
>>        Bug#58190 BETWEEN no longer uses indexes for date or datetime 
>> fields
>>
>>        Regression introduced by WL#2649.
>>
>>        Problem: queries with date/datetime columns did not use indexes:
>>
>>        set names non_latin1_charset;
>>        select * from date_index_test
>>        where date_column between '2010-09-01' and '2010-10-01';
>>
>>        before WL#2649 indexes worked fine because charset of 
>> date/datetime
>>        columns was BINARY which always won.
>>
>>        Fix: testing that collation of the operation matches collation 
>> of the field
>>        is only needed in case of "real" string data types.
>>        For DATE, DATETIME it's not needed.
> 
> Please consider adding the following text to CS comment:
> -----------------------------------------------------------
> The implementation is the following: a new property
> (like result_type() for instance) of Field hierarchy is introduced:
> match_collation_to_optimize_range(). That's a boolean flag that
> indicates if a Field object is an instance of Field_string or 
> Field_varstring, or Field_blob, or not.

Added this comment (with somewhat different wording)

> 
> It's used in opt_range.cc only. It should be considered "experimental"
> and should not be used elsewhere.

I am not really sure it will be removed with the patch for bug#58329,
so it will likely stay in the code for a while (until we have separate
type codes like DATE_RESULT, TIME_RESULT, etc). So it's not really
experimental.

> -----------------------------------------------------------
> 
>>        Note, the problem remains with UCS2, UTF16, UTF32 (index is not 
>> used).
>>        But this is not a regressions, as in 5.1 comparison between
>>        DATE/DATETIME and UCS2/UTF16/UTF32 did not work at all (always 
>> returned FALSE).
> 
> Please consider adding the following text to CS comment:
> -----------------------------------------------------------
> Another bug will be submitted to resolve issues with UCS2, UTF16 and
> UTF32; Field::match_collation_to_optimize_range() will be removed
> in scope of that bug.

Mentioned bug#58329 in the commit comments.

> -----------------------------------------------------------
> 
>> === modified file 'mysql-test/include/ctype_numconv.inc'
>> --- a/mysql-test/include/ctype_numconv.inc    2010-10-06 12:15:59 +0000
>> +++ b/mysql-test/include/ctype_numconv.inc    2010-11-19 14:35:02 +0000
>> @@ -1723,6 +1723,21 @@ DROP TABLE t1;
>>
>>
>>   --echo #
>> +--echo # Bug#58190 BETWEEN no longer uses indexes for date or 
>> datetime fields
>> +--echo #
>> +SELECT @@collation_connection;
> 
> Please, consider adding:
> -----------------------------------------------------------
> --disable_warnings
> DROP TABLE t1 IF EXISTS;
> --enable_warnings
> -----------------------------------------------------------

Don't need this. The previous test in ctype_numconv.inc drops t1.

> 
>> === modified file 'sql/field.h'
>> --- a/sql/field.h    2010-10-06 17:20:18 +0000
>> +++ b/sql/field.h    2010-11-19 14:35:02 +0000
>> @@ -243,6 +243,7 @@ public:
>>     virtual bool binary() const { return 1; }
>>     virtual bool zero_pack() const { return 1; }
>>     virtual enum ha_base_keytype key_type() const { return 
>> HA_KEYTYPE_BINARY; }
> 
> Please, consider adding:
> -----------------------------------------------------------
> /**
>   Indicates if the object is an instance Field_string or
>   Field_varstring, or Field_blob, or not. Please don't use it since
>   it will be removed eventually.
> */

Added extended comments before
  "virtual bool match_collation_to_optimize_range() const=0;",


> -----------------------------------------------------------
>> +  virtual bool match_collation_to_optimize_range() const { return 
>> FALSE; }
>>     virtual uint32 key_length() const { return pack_length(); }
>>     virtual enum_field_types type() const =0;
>>     virtual enum_field_types real_type() const { return type(); }
>>
>> === modified file 'sql/opt_range.cc'
>> --- a/sql/opt_range.cc    2010-09-16 12:20:35 +0000
>> +++ b/sql/opt_range.cc    2010-11-19 14:35:02 +0000
>> @@ -5795,7 +5795,7 @@ get_mm_leaf(RANGE_OPT_PARAM *param, COND
>>          WHERE latin1_swedish_ci_colimn = BINARY 'a '
>>
>>     */
> 
> Please, consider adding:
> -----------------------------------------------------------
> /*
>   The following "if" should succeed for "real" string fields only
>   (see comment on Field::match_collation_to_optimize_range()).
> */
> -----------------------------------------------------------
> 
>> -  if (field->result_type() == STRING_RESULT&&
>> +  if (field->match_collation_to_optimize_range()&&
> 
> Please add ' ' before &&

I did have ' ' before &&. It seems something went wrong
when you copied-and-pasted.