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

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 &&