Hi Martin
The proposed patch implements a separate phase for semantic checks,
that works with the current structure of the parsed tree.
If the parsed tree structure was adequate, this would be the logical
solution,
but in this case, I think that part of the problem is the structure of
the parsed tree itself,
which does not represent correctly the SQL_CACHE / SQL_NO_CACHE attribute.
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 (which forces to later explore the tree and
aggregate the result,
hence the proposed semantic check after parsing).
Instead of fixing the code to work with the current LEX data structures,
we could also fix the data structures to be more meaningful, and then
fix the parsing code to produce the right data.
Could you investigate if the following is a viable fix:
1) Keep all the tests written for this patch, they are all good.
2) Wait until bug#35020 is pushed, since it affects this area of the code
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 ...
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.
4) Change the way SQL_CACHE / SQL_NO_CACHE is represented in the parsed tree
The attribute in 'Lex->select_lex.sql_cache' should stay unchanged,
since it's here to preserve the *syntax*
(it's used to re-print the statement)
The attribute in 'Lex->select_lex.options|= OPTION_TO_QUERY_CACHE'
should *not* be
used to implement the CACHE / NO_CACHE property, since this property is
global to the statement,
not local to a select_lex.
Instead, define an attribute like :
Lex->stmt_options, with bit flags like OPTION_SQL_CACHE /
OPTION_SQL_NO_CACHE
Since there is now a unique Lex->stmt_options field,
there is no need to traverse the tree to find incompatible SQL_CACHE /
SQL_NO_CACHE usage,
and this test can be performed while parsing the SQL_CACHE /
SQL_NO_CACHE keywords
in the select_options rule.
Feel free to call me to discuss the details, or compare the pro / con of
each solution.
Thanks for investigating this path.
Regards,
Marc
mhansson@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 repository of mhansson. When mhansson does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2008-03-11 15:05:53+01:00, mhansson@riffraff.(none) +5 -0
> Bug#35138: illegal sql_cache select syntax
>
> The options SQL_CACHE and SQL_NO_CACHE were allowed in combination in
> any SELECT queries, where the use of one should deny
> use of the other.
> Fixed by adding flags to the parse tree and a semantic check after parsing.
>
> mysql-test/r/select.result@stripped, 2008-03-11 15:05:51+01:00,
> mhansson@riffraff.(none) +26 -0
> Bug#35138: Test result.
>
> mysql-test/t/select.test@stripped, 2008-03-11 15:05:51+01:00, mhansson@riffraff.(none)
> +33 -0
> Bug#35138: Test case.
>
> sql/mysql_priv.h@stripped, 2008-03-11 15:05:51+01:00, mhansson@riffraff.(none) +3 -1
> Bug#35138: New type for SELECT clause options bitmap. New #define's for
> bit indexes.
>
> sql/sql_parse.cc@stripped, 2008-03-11 15:05:51+01:00, mhansson@riffraff.(none) +55 -0
> Bug#35138: Added semantics checking for SELECT options statements.
> This could work as a hook for any semantic checking that is not
> dependent on anything but the parse tree.
>
> sql/sql_yacc.yy@stripped, 2008-03-11 15:05:51+01:00, mhansson@riffraff.(none) +2 -7
> Bug#35138: Moved semantics check for DISTINCT to after parsing.
> Added setting of SQL_CACHE/SQL_NO_CACHE flags.
>
>
