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.
