From: Martin Hansson
Date: December 30 2010 2:16pm
Subject: Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3537)
 Bug#48916
List-Archive: http://lists.mysql.com/commits/127719
Message-Id: <4D1C943E.9010305@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Sergey, patch approved, just some small things.

Sergey Glukhov wrote:
> #At file:///home/gluh/MySQL/mysql-5.1-bugteam/ based on revid:dao-gang.qu@stripped
>
>  3537 Sergey Glukhov	2010-12-30
>       Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
>       Before sorting HAVING condition is split into two parts,
>       first part is a table related condition and the rest of is
>       HAVING part. Extraction of HAVING part does not take into account
>       the fact that some of conditions might be non-const but
>       have 'used_tables' == 0 (undependent subqueries)
>   
Still "undependent" ;-)
>       and because of that these conditions are cut off by
>       make_cond_for_table() function.
>       The fix is to use (table_map) 0 instead of used_tables in
>       third argument for make_cond_for_table() function.
>       It allows to extract elements which belong to sorted
>       table and in addition elements which are undependend
>   
Also still misspelled ;-)
>       subqueries.
>      @ mysql-test/r/having.result
>         test result
>      @ mysql-test/t/having.test
>         test case
>      @ sql/sql_select.cc
>         The fix is to use (table_map) 0 instead of used_tables in
>         third argument for make_cond_for_table() function.
>         It allows to extract elements which belong to sorted
>         table and in addition elements which are undependend
>   
Also still misspelled ;-)
>         subqueries.
>
>     modified:
>       mysql-test/r/having.result
>       mysql-test/t/having.test
>       sql/sql_select.cc
> === modified file 'mysql-test/r/having.result'
> --- a/mysql-test/r/having.result	2010-07-09 10:39:47 +0000
> +++ b/mysql-test/r/having.result	2010-12-30 11:21:31 +0000
> @@ -545,4 +545,26 @@ FROM t1 JOIN t2 ON t2.f2 LIKE 'x'
>  HAVING field1 < 7;
>  field1
>  DROP TABLE t1,t2;
> +#
> +# Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
> +#
> +CREATE TABLE t1 (f1 INT, f2 INT);
> +INSERT INTO t1 VALUES (1, 0), (2, 1), (3, 2);
> +CREATE TABLE t2 (f1 INT, f2 INT);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT 4, 2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +SELECT t1.f1
> +FROM t1
> +HAVING 2 IN (SELECT f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +DROP TABLE t1,t2;
>  End of 5.1 tests
>
> === modified file 'mysql-test/t/having.test'
> --- a/mysql-test/t/having.test	2010-07-09 10:39:47 +0000
> +++ b/mysql-test/t/having.test	2010-12-30 11:21:31 +0000
> @@ -564,4 +564,30 @@ HAVING field1 < 7;
>  
>  DROP TABLE t1,t2;
>  
> +--echo #
> +--echo # Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
> +--echo #
> +
> +CREATE TABLE t1 (f1 INT, f2 INT);
> +INSERT INTO t1 VALUES (1, 0), (2, 1), (3, 2);
> +CREATE TABLE t2 (f1 INT, f2 INT);
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT 4, 2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING 2 IN (SELECT f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +DROP TABLE t1,t2;
> +
> +
>  --echo End of 5.1 tests
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-12-28 23:47:05 +0000
> +++ b/sql/sql_select.cc	2010-12-30 11:21:31 +0000
> @@ -2207,7 +2207,7 @@ JOIN::exec()
>  
>        Item* sort_table_cond= make_cond_for_table(curr_join->tmp_having,
>  						 used_tables,
> -						 used_tables);
> +						 (table_map) 0);
>        if (sort_table_cond)
>        {
>  	if (!curr_table->select)
> @@ -12814,6 +12814,41 @@ static bool test_if_ref(Item_field *left
>    return 0;					// keep test
>  }
>  
> +/**
> +   Extract a condition that can be checked after reading given table
> +
> +   @param cond       Condition to analyze
> +   @param tables     Tables for which "current field values" are available
> +   @param used_table Table that we're extracting the condition for (may
> +                     also include PSEUDO_TABLE_BITS, and may be zero)
> +   @param exclude_expensive_cond  Do not push expensive conditions
> +
> +   @retval <>NULL Generated condition
> +   @retval = NULL Already checked, OR error
>   
As I said to Roy earlier, can you please remove the space between = and 
NULL above? It looks very weird in Doxygen.
> +
> +   @details
> +     Extract the condition that can be checked after reading the table
> +     specified in 'used_table', given that current-field values for tables
> +     specified in 'tables' bitmap are available.
> +     If 'used_table' is 0
> +     - extract conditions for all tables in 'tables'.
> +     - extract uncorrelated conditions(i.e. conditions which have
> +       used_tables == 0).
>   
Aren't you confusing correlated sub-queries with conditions that don't 
refer to tables within the query block? IMU conditions that have 
used_tables == 0 either reference no tables at all or they reference 
sub-query tables and it doesn't matter if the sub-query is correlated or 
not. Correlation is a property of the sub-query, not the parent query.


Great patch!

Best Regards and a happy new year,

Martin