From: Martin Hansson
Date: March 14 2008 9:22am
Subject: Re: bk commit into 6.0 tree (mhansson:1.2609) BUG#35138
List-Archive: http://lists.mysql.com/commits/43980
Message-Id: <47DA43EE.2000502@mysql.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Marc,
I totally agree with you and will do pretty much what you prescribe. See 
below for some additianal thought and reflections.

/Martin

Marc Alff wrote:

>
> The SQL_CACHE / SQL_NO_CACHE property is a property of the entire 
> statement,
> so it should be represented by a single attribute in the LEX 
> structure, and not spread
> in the select_lex structures
Good point. I guess I got fooled by the current data structure. If we 
had a *pure* parse tree structure, then I think it would be ok to put 
the SQL_CACHE property on the node representing the SELECT statement 
that contains the SQL_CACHE token because the token is in that statement 
*syntactically*.
> 1) Keep all the tests written for this patch, they are all good.
Well, that depends on the code. :-) They test the *current* code 
optimally, but may have to change later.
>
> 2) Wait until bug#35020 is pushed, since it affects this area of the code
Sure. This patch will take some time to do right while 35020 is a 
no-brainer. So it will no doubt get pushed first. In fact, it has to.
>
> 3) Move the check for ALL / DISTINCT from the 'select_options: 
> select_option_list' rule,
> to the new rules:
> - 'query_expression_option: ALL' : when parsing ALL, test for the 
> DISTINCT flag ...
> - 'query_expression_option: DISTINCT' : when parsing DISTINCT test for 
> the ALL flag ...
Good idea. But this will have to wait until 35020 is pushed obviously.
>
> This way, the ALL / DISTINCT test will be available for both select 
> and sub selects
>
> Since the semantic checks for ALL / DISTINCT are local to a select_lex,
> these can be done during parsing, there is no need to delay this check 
> to after parsing.
>
> 3-b) Also, add a test to cover this:
>
> SELECT ALL col1 from t1 ... UNION (SELECT DISTINCT col2 from t2 ...)
>
> which should be legal, since there are 2 independent 
> query_expression_option_list here.
In fact I think that even

SELECT ALL col1 from t1 ... UNION SELECT DISTINCT col2 from t2 ...

is allowed. Of course this is semantically meaningless since UNION 
discards duplicates anyway, but we are talking syntax here. Otoh I think 
we *have to* allow the following since it works now, is meaningful, and 
is most likely in use.

SELECT ALL col1 from t1 ... UNION ALL SELECT DISTINCT col2 from t2 ...

But of course nothing similar will be allowed for SQL_CACHE/SQL_NO_CACHE...
>
> 4) Change the way SQL_CACHE / SQL_NO_CACHE is represented in the 
> parsed tree
>
> [...]
>
> Instead, define an attribute like :
>
> Lex->stmt_options, with bit flags like OPTION_SQL_CACHE / 
> OPTION_SQL_NO_CACHE
Excellent idea! This will settle the notion that SELECT_LEX is in fact 
an abstract syntax tree, not a parse tree.