List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:August 4 2010 3:00pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (martin.hansson:3203)
Bug#33062
View as plain text  
Roy Lyseng wrote:
> Hi Martin,
>
> thanks for doing this cleanup!
>
> On 04.08.10 14.22, Martin Hansson wrote:
>> ...
>> @@ -10069,11 +10066,11 @@ uint check_join_cache_usage(JOIN_TAB *ta
>>       if ((rows != HA_POS_ERROR)&&  !(flags& 
> HA_MRR_USE_DEFAULT_IMPL)&&
>>           (!(flags&  HA_MRR_NO_ASSOCIATION) || cache_level>  6)&&
>>           ((options&  SELECT_DESCRIBE) ||
>> -         (cache_level<= 6&&
>> -          (tab->cache= new JOIN_CACHE_BKA(join, tab, flags, 
>> prev_cache)) ||
>> -      cache_level>  6&&
>> -          (tab->cache= new JOIN_CACHE_BKA_UNIQUE(join, tab, flags, 
>> prev_cache))
>> -         )&&  !tab->cache->init()))
>> +         (((cache_level<= 6&&
>> +            (tab->cache= new JOIN_CACHE_BKA(join, tab, flags, 
>> prev_cache))) ||
>> +           (cache_level>  6&&
>> +            (tab->cache= new JOIN_CACHE_BKA_UNIQUE(join, tab, flags, 
>> prev_cache)))
>> +           )&&  !tab->cache->init())))
>
> It seems OK, although IMHO it is too convoluted and should be refactored.
Yes, and it should have been done when implementing BKA. How an 
implementor gets away with cluttering up the code to this level is 
beyond me.
>>       {
>>         if (cache_level<= 6)
>>           return JOIN_CACHE::ALG_BKA | force_unlinked_cache;
>> @@ -15269,49 +15266,6 @@ err:
>>     DBUG_RETURN(NULL);                /* purecov: inspected */
>>   }
>>
>> -/**
>> -   @brief Replaces an expression destructively inside the expression 
>> tree of
>> -   the WHERE clase.
>> -
>> -   @note Because of current requirements for semijoin flattening, we 
>> do not
>> -   need to recurse here, hence this function will only examine the 
>> top-level
>> -   AND conditions. (see JOIN::prepare, comment above the line
>> -   'if (do_materialize)'
>> -
>> -   @param join The top-level query.
>> -   @param old_cond The expression to be replaced.
>> -   @param new_cond The expression to be substituted.
>> -   @param do_fix_fields If true, Item::fix_fields(THD*, Item**) is 
>> called for
>> -   the new expression.
>> -   @return<code>true</code>  if there was an 
>> error,<code>false</code>  if
>> -   successful.
>> -*/
>> -static bool replace_where_subcondition(JOIN *join, Item *old_cond,
>> -                                       Item *new_cond, bool 
>> do_fix_fields)
>> -{
>> -  if (join->conds == old_cond) {
>> -    join->conds= new_cond;
>> -    if (do_fix_fields)
>> -      new_cond->fix_fields(join->thd,&join->conds);
>> -    return FALSE;
>> -  }
>> -
>> -  if (join->conds->type() == Item::COND_ITEM) {
>> -    List_iterator<Item>  
>> li(*((Item_cond*)join->conds)->argument_list());
>> -    Item *item;
>> -    while ((item= li++))
>> -      if (item == old_cond)
>> -      {
>> -        li.replace(new_cond);
>> -        if (do_fix_fields)
>> -          new_cond->fix_fields(join->thd, li.ref());
>> -        return FALSE;
>> -      }
>> -  }
>> -
>> -  return TRUE;
>> -}
>> -
>>   /*
>>     Create a temporary table to weed out duplicate rowid combinations
>>
>> @@ -18336,39 +18290,47 @@ static bool test_if_ref(COND *root_cond,
>>
>>
>>   /**
>> -   @brief Replaces an expression destructively inside the expression 
>> tree of
>> -   the WHERE clase.
>> +   Destructively replaces a sub-expression inside an expression 
>> tree. The
>> +   parse tree is also altered.
>
> Please replace "expression" with "condition" (function name is 
> replace_subcondition)
done

>
>> +
>>      @param old_cond The expression to be replaced.
>> +
>>      @param new_cond The expression to be substituted.
>> +
>>      @param do_fix_fields If true, Item::fix_fields(THD*, Item**) is 
>> called for
>>      the new expression.
>> -   @return<code>true</code>  if there was an 
>> error,<code>false</code>  if
>> -   successful.
>> +
>> +   @return error status
>> +
>> +   @retval true If there was an error during prerequisites check.
>
> true If an error occurred
>
> (there is no prerequisite check here - an assert is thrown if input is 
> bad)
Assert is thrown only in debug builds. The prerequisite is *tree == 
old_cond || (*tree)->type() == Item::COND_ITEM. I realize that was very 
unclear so I check it up front instead.
>
>> ...
>
> Missing return value check on fix_fields()
ok, fixed.
>
>> +    join->select_lex->where= *tree;
>
> I wonder if this last line should be:
>
>        if (old_cond == join->select_lex->where)
>          join->select_lex->where= *tree;
>
> I do not know what should happen if the if () is false, though...
Let's leave it for now.
>
>>       return FALSE;
>>     }
>>
>> -  if ((*expr)->type() == Item::COND_ITEM)
>> +  if ((*tree)->type() == Item::COND_ITEM)
>>     {
>> -    List_iterator<Item>  li(*((Item_cond*)(*expr))->argument_list());
>> +    List_iterator<Item>  li(*((Item_cond*)(*tree))->argument_list());
>>       Item *item;
>>       while ((item= li++))
>>       {
>> @@ -18376,7 +18338,8 @@ static bool replace_where_subcondition(J
>>         {
>>           li.replace(new_cond);
>>           if (do_fix_fields)
>> -          new_cond->fix_fields(join->thd, li.ref());
>> +          if (new_cond->fix_fields(join->thd, li.ref()))
>> +            return TRUE;
>>           return FALSE;
>>         }
>>       }
>
> Suggestion:
>
> You might structure the function like this:
>   if (*tree == old_cond)
>     ..
>   else if ((*tree)->type() == Item::COND_ITEM)
>     ...
>   else
>     DBUG_ASSERT(FALSE);
>
>   return FALSE;
>
> You get a logical if - else if - else structure, and you get rid of 
> the inline return FALSE statements, leaving only the inline error 
> return statements.
That would force me to use a break statement and I don't like them. A 
return is much easier to follow in a debugger.

Yours,

Martin

Thread
bzr commit into mysql-next-mr-opt-backporting branch (martin.hansson:3203)Bug#33062Martin Hansson4 Aug
Re: bzr commit into mysql-next-mr-opt-backporting branch(martin.hansson:3203) Bug#33062Roy Lyseng4 Aug
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (martin.hansson:3203)Bug#33062Martin Hansson4 Aug
Re: bzr commit into mysql-next-mr-opt-backporting branch(martin.hansson:3203) Bug#33062Roy Lyseng4 Aug