From: Roy Lyseng
Date: October 22 2010 12:52pm
Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3265)
 Bug#45174 Bug#50019 Bug#52068
List-Archive: http://lists.mysql.com/commits/121692
Message-Id: <4CC1890C.4080501@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Guilhem,

On 22.10.10 11.33, Guilhem Bichot wrote:
> Hello,
>
> Roy Lyseng a écrit, Le 22.10.2010 09:58:
>> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
>> revid:guilhem@stripped
>>
>> 3265 Roy Lyseng 2010-10-22
>> Bug#45174: Incorrectly applied equality propagation caused wrong result
>> on a query with a materialized semi-join.
>> Bug#50019: Wrong result for IN-query with materialization.
>> Bug#52068: Optimizer generates invalid semijoin materialization plan
>
>> === modified file 'sql/item_cmpfunc.h'
>> --- a/sql/item_cmpfunc.h 2010-10-15 10:32:50 +0000
>> +++ b/sql/item_cmpfunc.h 2010-10-22 07:57:40 +0000
>> @@ -1637,7 +1637,13 @@ public:
>> void add(Item_field *f);
>> uint members();
>> bool contains(Field *field);
>> + /**
>> + Get the first field of multiple equality, use for semantic checking.
>> +
>> + @retval First field in the multiple equality.
>
> indentation: Get and @retval should move to the right.

Fixed
>
>> === modified file 'sql/sql_select.cc'
>
>> @@ -12563,57 +12619,42 @@ Item *eliminate_item_equal(Item *cond, C
>> {
>> if (eq_item)
>> eq_list.push_back(eq_item);
>> - /*
>> - item_field might refer to a table that is within a semi-join
>> - materialization nest. In that case, the join order looks like this:
>>
>> - outer_tbl1 outer_tbl2 SJM (inner_tbl1 inner_tbl2) outer_tbl3 + /*
>> + item_field may refer to a table that is within a semijoin
>> + materialization nest. In that case, the join order may look like:
>>
>> - We must not construct equalities like + ot1 ot2 SJM (it3 it4) ot5
>> - outer_tbl1.col = inner_tbl1.col + If we have a multiple equality (ot1.c1,
>> ot2.c2, it3.c3, it4.c4, ot5.c5),
>> + we should generate the following equalities:
>> + 1. ot1.c1 = ot2.c2
>> + 2. ot1.c1 = it1.c3
>> + 3. it1.c3 = it2.c4
>
> but it1 and it2 are named it3 and it4 now

Thanks
>
>> + 4. ot1.c1 = ot5.c5
>> +
>> + Equalities 1) and 4) are regular equalities between two outer tables.
>> + Equality 2) is an equality that matches the outer query with a
>> + materialized semijoin table. It is either performed as a lookup
>> + into the materialized table (SJM-lookup), or as a condition on the
>> + outer table (SJM-scan).
>> + Equality 3) is evaluated during semijoin materialization.
>> +
>> + If there is a const item, match against this one.
>> + Otherwise, match against the first field item in the multiple equality,
>> + unless the item is within a materialized semijoin nest, where we match
>> + against the first item within the SJM nest (if the item is not the first
>> + item within the SJM nest), or match against the first item in the
>> + list (if the item is the first one in the SJM list).
>> + */
>> + head= item_const ? item_const : item_equal->get_first(item);
>> + if (head == item)
>
> Optional: I would add "// first item in SJM nest"
> to the right of this if().

Done
>
>> @@ -18865,12 +18907,12 @@ static bool replace_subcondition(JOIN *j
>> guaranteed to be true by employed 'ref' access methods (the code that
>> does this is located at the end, search down for "EQ_FUNC").
>>
>> -
>> - SEE ALSO + @see
>> make_cond_for_info_schema uses similar algorithm
>>
>> - RETURN
>> - Extracted condition
>> + @note
>> + Make sure to keep the implementations of make_cond_for_table() and
>> + make_cond_after_sjm() synchronized.
>
> Optional: move the "@see make_cond_for_info_schema" into this @note (as another
> function to keep in sync).

Done
>
>> @@ -18963,20 +19011,26 @@ make_cond_for_table_from_pred(Item *root
>> }
>>
>> /*
>> - Because the following test takes a while and it can be done
>> - table_count times, we mark each item that we have examined with the result
>> - of the test
>> - */
>> + Omit this condition if
>> + 1. It has been marked as omittable before, or
>> + 2. Some tables referred by the condition are not available, or
>> + 3. We are extracting conditions for any tables, the condition is
>
> Optional: any -> all

Done
>
>> @@ -19030,24 +19084,35 @@ make_cond_for_table_from_pred(Item *root
>> @param sjm_tables Tables within the semi-join nest (the inner part).
>>
>> @retval <>NULL Generated condition
>> - @retval = NULL Already checked, or error
>> + @retval = NULL Already checked, OR error
>>
>> @details
>> A regular semi-join materialization is always non-correlated, ie
>> the subquery is always resolved by performing a lookup generated in
>> - create_subquery_equalities, hence this function should never produce
>> + create_subquery_equalities, hence this function never needs to produce
>> any condition for regular semi-join materialization.
>
> optional: "regular semi-join materialization" could be changed to "semi-join
> materialization with lookup". There's nothing specially "regular" with it.

Done.
>
> Ok to push. Really good work. I learnt quite a bit.