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.
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.
It's used in opt_range.cc only. It should be considered "experimental"
and should not be used elsewhere.
-----------------------------------------------------------
> 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.
-----------------------------------------------------------
> === 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
-----------------------------------------------------------
> === 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.
*/
-----------------------------------------------------------
> + 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 &&
