List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 1 2011 7:19am
Subject:Re: bzr commit into mysql-trunk branch (alexander.barkov:3302) Bug#58329
Bug#11765369
View as plain text  
Hello Bar,

Alexander Barkov a écrit, Le 18.03.2011 13:46:
> #At file:///home/bar/mysql-bzr/mysql-trunk.b58329/ based on
> revid:alexander.nozdrin@stripped
> 
>  3302 Alexander Barkov	2011-03-18
>       Bug#11765369 - Bug#58329: BETWEEN DOES NOT USE INDEXES FOR DATE OR DATETIME
> FIELDS FOR UCS2+ 
>       Problem:
>         WHERE date_column = 'ucs2 literal'
>       was rewritten to
>         WHERE CONVERT(date_column USING ucs2) = 'ucs2 literal'
>       so index on date_column did not work any more.
>       
>       Fix: rewrite to
>         WHERE date_column = CONVERT('ucs2 literal' USING latin1)

Without the patch: why did we choose to convert date_column instead of 
the literal? There was probably a rule somewhere?

Why is the bug specific of UCS2?

> === modified file 'mysql-test/t/ctype_ucs.test'
> --- a/mysql-test/t/ctype_ucs.test	2011-03-04 14:59:32 +0000
> +++ b/mysql-test/t/ctype_ucs.test	2011-03-18 12:43:19 +0000
> @@ -779,5 +779,15 @@ DO IFNULL(CHAR(NULL USING ucs2), '');
>  DO CAST(CONVERT('' USING ucs2) AS UNSIGNED);
>  
>  --echo #
> +--echo # Bug#11765369 - 58329: BETWEEN DOES NOT USE INDEXES FOR DATE OR DATETIME
> FIELDS FOR UCS2+
> +--echo #
> +SET NAMES utf8, character_set_connection=ucs2;
> +CREATE TABLE t1 (id INT DEFAULT NULL, date_column DATE DEFAULT NULL, KEY
> (date_column)) engine=myisam;

I'd delete "=engine=myisam"; so that the test can also run in InnoDB, in 
case one starts MTR with --mysqld=--default-storage-engine=innodb

> +INSERT INTO t1 VALUES (1,'2010-09-01'), (2,'2010-10-01');
> +EXPLAIN EXTENDED SELECT * FROM t1 WHERE date_column= '2010-09-01';
> +DROP TABLE t1;
> +SET NAMES latin1;
> +
> +--echo #
>  --echo # End of 5.6 tests
>  --echo #
> 
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2011-03-17 11:33:17 +0000
> +++ b/sql/item_cmpfunc.cc	2011-03-18 12:43:19 +0000
> @@ -484,10 +484,31 @@ void Item_bool_func2::fix_length_and_dec
>    
>    DTCollation coll;
>    if (args[0]->result_type() == STRING_RESULT &&
> -      args[1]->result_type() == STRING_RESULT &&
> -      agg_arg_charsets_for_comparison(coll, args, 2))
> -    return;
> -    
> +      args[1]->result_type() == STRING_RESULT)
> +  {
> +    if (args[0]->is_datetime() || args[1]->is_datetime())
> +    {
> +      /*
> +        For DATE/TIME we always convert to my_charset_numeric (latin1),
> +        to make this:
> +          WHERE date_column = 'ucs2 literal';
> +
> +        rewrite to
> +          WHERE date_column = CONVERT('ucs2 literal' USING latin1);
> +
> +        instead of 
> +          WHERE CONVERT(date_column USING ucs2) = 'ucs2 literal';
> +
> +        This makes it possible to use index on date_column for optimization.
> +      */
> +      coll.set(&my_charset_numeric);
> +      if (agg_item_set_converter(coll, func_name(), args, 2, 0, 1))
> +        return;
> +    }
> +    else if (agg_arg_charsets_for_comparison(coll, args, 2))
> +      return;
> +  }

I worry to have one agg* function called for DATE/TIME and another one 
called for other types, i.e. I worry about "if/else". I feel that this 
could introduce DATE/TIME specific code paths, and thus bugs in the future.
It looks like agg_arg_charsets_for_comparison() calls 
agg_item_set_converter(), so can't we do this instead, to avoid if/else:

 >    if (args[0]->result_type() == STRING_RESULT &&
 > +      args[1]->result_type() == STRING_RESULT)
 > +  {
 > +    if (args[0]->is_datetime() || args[1]->is_datetime())
 > +    {
 > +      /*
 > +        For DATE/TIME we always convert to my_charset_numeric (latin1),
 > +        to make this:
 > +          WHERE date_column = 'ucs2 literal';
 > +
 > +        rewrite to
 > +          WHERE date_column = CONVERT('ucs2 literal' USING latin1);
 > +
 > +        instead of
 > +          WHERE CONVERT(date_column USING ucs2) = 'ucs2 literal';
 > +
 > +        This makes it possible to use index on date_column for 
optimization.
 > +      */
 > +      coll.set(&my_charset_numeric);
 > +    }
 > +    if (agg_arg_charsets_for_comparison(coll, args, 2))
 > +      return;
 > +  }

?
Thread
bzr commit into mysql-trunk branch (alexander.barkov:3302) Bug#58329Bug#11765369Alexander Barkov18 Mar
  • Re: bzr commit into mysql-trunk branch (alexander.barkov:3302) Bug#58329Bug#11765369Guilhem Bichot1 Apr
    • Re: bzr commit into mysql-trunk branch (alexander.barkov:3302) Bug#58329Bug#11765369Alexander Barkov1 Apr
      • Re: bzr commit into mysql-trunk branch (alexander.barkov:3302) Bug#58329Bug#11765369Guilhem Bichot4 Apr