Hello Sanja, Igor, This patch removes a lot of main_select_push() / pop_select() that you had to add into *.yy files in the 10.3-MDEV-11953 branch. The patch does the following: - Disallows creation of Item_field in the select stack is empty. An error is returned instead (ER_BAD_FIELD_ERROR). The point is that Item_field requires a Name_resolution_context, which is not available with the empty select stack. This additionally fixed the problem reported in: MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable - Disallows creation of Item_sum and Item_windowfunc the same way, as they also need a Name_resolution_context. It fixed the second part of MDEV-14347, about aggregate and window functions. It also fixed: MDEV-15870 Using aggregate and window function in unexpected places can crash the server (repeatable since 10.2) - Adds FOR_LOOP_BOUND into enum_parsing_place, to force creation of a temporary Item_field when inside a FOR loop: FOR x IN ident If the "ident" is later resolves to a cursor name, then the loop is treated as "explicit cursor FOR loop", otherwise it's a normal field name and therefore the same error (ER_BAD_FIELD_ERROR) is returned. - Adds a new method LEX::create_item_query_expression() to reuse the same code in sql_yacc.yy and sql_yacc_ora.yy. Adds a test for curr_sel, in case of NULL resets curr_sel to &lex->builtin_select. - Adds tests to cover IN and EXISTS subselects in various query parts. This was very weakly covered. While working on this patch I had problems with IN and EXISTS subselects, so I had to cover them. - Adds a function relink_hack() to properly bind st_select_lex containing an IN/EXISTS subselect to thd->lex->builtin_select. This is needed for the cases when the statement does not do main_select_push(). I does not know this code well, so you can fine a better solution. Please suggest. I propose the following plan: 1. Move tests for IN/EXISTS from this patch just to 10.3 and rebase your branch. 2. Please review the remaining main_select_push()/pop_select(). Note, these rules must have push/pop for sure, as they need Item_field as a part of their syntax: insert: update: delete: show: handler: But some of push/pop pairs should also be further removed: - Some rules can probably assume (and assert) that the select stack is not empty, as the parent rule must have pushed an entry earlier: partition_entry: parse_vcol_expr: single_multi: - Non-TABLE related entries in: create: alter: cannot use Item_field (e.g CREATE DATABASE or ALTER DATABASE), so push/pop can be removed. - I'm not sure about load: set: I can study these separately (if we generally agree on the plan). 3. Move the part of the patch fixing MDEV-15870 and MDEV-14347 to 10.4, so then you rebase on top of it. Alternatively, just keep MDEV-15870 and MDEV-14347 as a part of 10.3-MDEV-11953. Please decide. On 04/10/2018 03:37 PM, Oleksandr Byelkin wrote:
Am 10.04.2018 um 10:58 schrieb Alexander Barkov:
Hello Sanja,
I reviewed your recent changes in "10.3-MDEV-11953" (and the attached additional patch for sql_yacc_ora.yy)
I have some proposals:
1. Can you please move huge pieces of the code from sql_yacc.yy to LEX or other relevant classes?
It makes the grammar much more readable (patches are aslo much more readable).
I'd move the relevant pieces of the code to LEX as a separate patch, even before fixing the grammar. OK
2. You're adding too many main_select_push() and pop_select(). Please move them to upper level rules (it should be possible in many cases). Impossible
Add new helper rules when needed. For example, this piece of code repeats many times:
+ { + if (Lex->main_select_push()) + MYSQL_YYABORT; + } + expr + { + Lex->pop_select(); //main select + $$= $3;
It deserved a rule, say, expr_with_select_push_pop. You can find a better name :)
OK
- Serg and I spent a lot of time working on this task: MDEV-8909 union parser cleanup (and its 13 dependency tasks, and 3 related tasks, see the "Issue links" section in MDEV).
We think that it should be the parser who disallows bad grammar, instead of post-analysis with raising errors like ER_CANT_USE_OPTION_HERE. Please keep using the same approach. The task did not made parser recognizing brackets, and I have no ideas how to return parser errors when all SELECT parsed in the same way in difference from the previous parser which could recognize only one level of SELECTs. Thanks!
[skip]