From: Sergey Petrunia
Date: November 18 2008 3:45pm
Subject: Re: bzr commit into mysql-6.0-opt branch (timour:2700) Bug#36752
List-Archive: http://lists.mysql.com/commits/59087
Message-Id: <20081118154506.GQ13327@pslp2.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7BIT

Hi,

On Wed, Nov 12, 2008 at 05:19:35PM +0200, Timour Katchaounov wrote:
> #At file:///home/tkatchaounov/mysql/bzr/6.0-b36752/
> 
>  2700 Timour Katchaounov	2008-11-12
>       BUG#36752 subquery materialization produces wrong results when comparing different types
>       

One small comment, please see below:

> === modified file 'sql/item_subselect.cc'
> --- a/sql/item_subselect.cc	2008-10-20 09:16:47 +0000
> +++ b/sql/item_subselect.cc	2008-11-12 15:19:22 +0000
> @@ -3110,11 +3110,54 @@ bool subselect_hash_sj_engine::init_perm
>    KEY_PART_INFO *cur_key_part= tmp_key->key_part;
>    store_key **ref_key= tab->ref.key_copy;
>    uchar *cur_ref_buff= tab->ref.key_buff;
> +
> +  /*
> +    Create an artificial condition to post-filter those rows matched by index
> +    lookups that cannot be distinguished by the index lookup procedure, e.g.
> +    because of truncation. Prepared statements execution requires that
> +    fix_fields is called for every execution. In order to call fix_fields we
> +    need to create a Name_resolution_context and a corresponding TABLE_LIST
> +    for the temporary table for the subquery, so that all column references
> +    to the materialized subquery table can be resolved correctly.
> +  */
> +  DBUG_ASSERT(cond == NULL);
> +  if (!(cond= new Item_cond_and))
> +    DBUG_RETURN(TRUE);
> +  /*
> +    Table reference for tmp_table that is used to resolve column references
> +    (Item_fields) to columns in tmp_table.
> +  */
> +  TABLE_LIST *tmp_table_ref;
> +  if (!(tmp_table_ref= (TABLE_LIST*) thd->calloc(sizeof(TABLE_LIST))))
> +    DBUG_RETURN(TRUE);
> +
> +  tmp_table_ref->init_one_table(NULL, 0, "materialized subselect", 22,
> +                                "materialized subselect", TL_READ);
> +  tmp_table_ref->table= tmp_table;
> +
> +  /* Name resolution context for all tmp_table columns created below. */
> +  Name_resolution_context *context= new Name_resolution_context;
> +  context->init();
> +  context->first_name_resolution_table=
> +    context->last_name_resolution_table= tmp_table_ref;
>    
>    for (uint i= 0; i < tmp_key_parts; i++, cur_key_part++, ref_key++)
>    {
> -    tab->ref.items[i]= item_in->left_expr->element_index(i);
> +    Item_func_eq *eq_cond; /* New equi-join condition for the current column. */
> +    /* Item for the corresponding field from the materialized temp table. */
> +    Item_field *right_col_item;
>      int null_count= test(cur_key_part->field->real_maybe_null());
> +    tab->ref.items[i]= item_in->left_expr->element_index(i);
> +
> +    if (!(right_col_item= new Item_field(thd, context, cur_key_part->field)) ||
> +        !(eq_cond= new Item_func_eq(tab->ref.items[i], right_col_item)) ||
> +        ((Item_cond_and*)cond)->add(eq_cond))
> +    {
> +      delete cond;
> +      cond= NULL;
> +      DBUG_RETURN(TRUE);
> +    }
> +
>      *ref_key= new store_key_item(thd, cur_key_part->field,
>                                   /* TODO:
>                                      the NULL byte is taken into account in
> @@ -3131,6 +3174,9 @@ bool subselect_hash_sj_engine::init_perm
>    tab->ref.key_err= 1;
>    tab->ref.key_parts= tmp_key_parts;
>  
> +  if (cond->fix_fields(thd, 0))
> +    DBUG_RETURN(TRUE);
> +
Please either 
- add a note in Item_cond::fix_field() that it must not try substituting
  itself for another item, or
- make the code conform to fix_fields()'s calling convention

>    DBUG_RETURN(FALSE);
>  }
>  
> @@ -3150,6 +3196,12 @@ bool subselect_hash_sj_engine::init_runt
>      the subquery if not yet created.
>    */
>    materialize_engine->prepare();
> +  /*
> +    Repeat name resolution for 'cond' since cond is not part of any
> +    clause of the query, and it is not 'fixed' during JOIN::prepare.
> +  */
> +  if (cond && !cond->fixed && cond->fix_fields(thd, 0))
> +    return TRUE;
Same here.

>    /* Let our engine reuse this query plan for materialization. */
>    materialize_join= materialize_engine->join;
>    materialize_join->change_result(result);

Ok to push after the above is addressed.

BR
 Sergey
-- 
Sergey Petrunia, Lead Software Engineer
MySQL AB, www.mysql.com
Office: N/A
Blog: http://s.petrunia.net/blog