Hi! Generally the patch made very good impression but lack of many tests which made reviewing more dificult (I made simple tests just to check basic functionality), naturally that test cowerage shoud be extended (see liist in comments). Also there are several small notes (except probably adding debug printing, which maybe is not very small) and one question in the following lines:
diff --git a/mysql-test/r/cte_nonrecursive.result b/mysql-test/r/cte_nonrecursive.result new file mode 100644 index 0000000..07449bc --- /dev/null +++ b/mysql-test/r/cte_nonrecursive.result @@ -0,0 +1,655 @@ +create table t1 (a int, b varchar(32)); +insert into t1 values +(4,'aaaa' ), (7,'bb'), (1,'ccc'), (4,'dd'); +insert into t1 values
[skip] I understand that most part works by inheritance from derived table and it really works, but better to have in the regression suite proof of working: HAVING ORDER BY... LIMIT I see that privileges check code is changed but I do not see test with priveleges check (I checked several cases and everything looks OK, but it shoud be good number different cases) - privilages on whole DB (given / not given) - same for table - same for field There is no tests of using CTE in stored procedures and prepared statements (again I checked some basics it looks like working). [skip]
--- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7136,3 +7136,11 @@ ER_KILL_QUERY_DENIED_ERROR eng "You are not owner of query %lu" ger "Sie sind nicht Eigentümer von Abfrage %lu" rus "Вы не являетесь владельцем запроса %lu" +ER_WITH_COL_WRONG_LIST + eng "With column list and SELECT field list have different column counts"
IMHO above it will be battter to write 'WITH' in capital letters.
+ER_DUP_QUERY_NAME + eng "Duplicate query name in with clause"
IMHO above it will be battter to write 'WITH' in capital letters.
+ER_WRONG_ORDER_IN_WITH_CLAUSE + eng "The definition of the table '%s' refers to the table '%s' defined later in a non-recursive with clause"
IMHO above it will be battter to write 'WITH' in capital letters.
+ER_RECURSIVE_QUERY_IN_WITH_CLAUSE + eng "Recursive queries in with clause are not supported yet"
It is better use ER_NOT_SUPPORTED_YET instead of above error message (especially taking into account that situation is temporar) [skip]
--- /dev/null +++ b/sql/sql_cte.cc @@ -0,0 +1,597 @@ +#include "sql_class.h" +#include "sql_lex.h" +#include "sql_cte.h" +#include "sql_view.h" // for make_valid_column_names +#include "sql_parse.h" + + +/** + @brief
it is no need to put @brief explicit, first lines supposed to be it.
+ Check dependencies between tables defined in a list of with clauses + + @param + with_clauses_list Pointer to the first clause in the list + + @details + The procedure just calls the method With_clause::check_dependencies + for each member of the given list. + + @retval + false on success + true on failure +*/ + +bool check_dependencies_in_with_clauses(With_clause *with_clauses_list) +{ + for (With_clause *with_clause= with_clauses_list; + with_clause; + with_clause= with_clause->next_with_clause) + { + if (with_clause->check_dependencies()) + return true; + } + return false; +} +
[skip]
+st_select_lex_unit *With_element::clone_parsed_spec(THD *thd, + TABLE_LIST *with_table) +{ + LEX *lex; + st_select_lex_unit *res= NULL; + Query_arena backup; + Query_arena *arena= thd->activate_stmt_arena_if_needed(&backup);
It is better to put DBUG_ENTER/DBUG_PRINT/DBUG_RETURN in the key methods/function like this otherwise debug log become unreadable, in this partucular case parser will be called 'out of the blue' in the log. I agree that the log should not be overloaded, but all big important events of query life should be reflected there. so it is better look through the wunctions again and pur debug logging in the one which are really important.
+ + if (!(lex= (LEX*) new(thd->mem_root) st_lex_local)) + { + if (arena) + thd->restore_active_arena(arena, &backup); + return res; + } + LEX *old_lex= thd->lex; + thd->lex= lex; + + bool parse_status= false; + Parser_state parser_state; + TABLE_LIST *spec_tables; + TABLE_LIST *spec_tables_tail; + st_select_lex *with_select; + + if (parser_state.init(thd, unparsed_spec.str, unparsed_spec.length)) + goto err; + lex_start(thd); + with_select= &lex->select_lex; + with_select->select_number= ++thd->select_number; + parse_status= parse_sql(thd, &parser_state, 0); + if (parse_status) + goto err; + spec_tables= lex->query_tables; + spec_tables_tail= 0; + for (TABLE_LIST *tbl= spec_tables; + tbl; + tbl= tbl->next_global) + { + tbl->grant.privilege= with_table->grant.privilege; + spec_tables_tail= tbl; + } + if (spec_tables) + { + if (with_table->next_global) + { + spec_tables_tail->next_global= with_table->next_global; + with_table->next_global->prev_global= &spec_tables_tail->next_global; + } + else + { + old_lex->query_tables_last= &spec_tables_tail->next_global; + } + spec_tables->prev_global= &with_table->next_global; + with_table->next_global= spec_tables; + } + res= &lex->unit; + + lex->unit.include_down(with_table->select_lex); + lex->unit.set_slave(with_select); + old_lex->all_selects_list= + (st_select_lex*) (lex->all_selects_list-> + insert_chain_before( + (st_select_lex_node **) &(old_lex->all_selects_list), + with_select)); + lex_end(lex); +err: + if (arena) + thd->restore_active_arena(arena, &backup); + thd->lex= old_lex; + return res; +} +
[skip]
diff --git a/sql/sql_cte.h b/sql/sql_cte.h new file mode 100644 index 0000000..1d57237 --- /dev/null +++ b/sql/sql_cte.h @@ -0,0 +1,180 @@ +#ifndef SQL_CTE_INCLUDED +#define SQL_CTE_INCLUDED +#include "sql_list.h" +#include "sql_lex.h" + +class With_clause; + +/** + @class With_clause + @brief Set of with_elements + + It has a reference to the first with element from this with clause. + This reference allows to navigate through all the elements of the with clause. + It contains a reference to the unit to which this with clause is attached. + It also contains a flag saying whether this with clause was specified as recursive. +*/ + +class With_element : public Sql_alloc +{ +private: + With_clause *owner; // with clause this object belongs to + With_element *next_elem; // next element in the with clause
Was you considering using List<> instead of direct link managing, it looks like new trend in MySQL. I am not for using them here but still interesting why not List<>? [skip]
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 7787768..ce09808 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -54,6 +54,7 @@ #include "sql_handler.h" // Sql_cmd_handler_* #include "sql_signal.h" #include "sql_get_diagnostics.h" // Sql_cmd_get_diagnostics +#include "sql_cte.h" #include "event_parse_data.h" #include "create_options.h" #include <myisam.h> @@ -63,6 +64,7 @@ #include "rpl_mi.h" #include "lex_token.h"
+
Remove please above empty line.
/* this is to get the bison compilation windows warnings out */ #ifdef _MSC_VER /* warning C4065: switch statement contains 'default' but no 'case' labels */ @@ -959,6 +961,8 @@ bool LEX::set_bincmp(CHARSET_INFO *cs, bool bin) class sp_label *splabel; class sp_name *spname; class sp_variable *spvar; + class With_clause *with_clause; + handlerton *db_type; st_select_lex *select_lex; struct p_elem_val *p_elem_value; @@ -1456,6 +1460,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); %token REAL /* SQL-2003-R */ %token REBUILD_SYM %token RECOVER_SYM +%token RECURSIVE_SYM
It it is not keyword (one can name other object like this) you also have to add it to one of this lists: 1) /* Keyword that we allow for identifiers (except SP labels) */ keyword: 2) /* * Keywords that we allow for labels in SPs. * Anything that's the beginning of a statement or characteristics * must be in keyword above, otherwise we get (harmful) shift/reduce * conflicts. */ keyword_sp: 3) /* Although a reserved keyword in SQL:2003 (and :2008), not reserved in MySQL per WL#2111 specification. */
%token REDOFILE_SYM %token REDO_BUFFER_SIZE_SYM %token REDUNDANT_SYM @@ -1740,6 +1745,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); case_stmt_body opt_bin_mod opt_if_exists_table_element opt_if_not_exists_table_element opt_into opt_procedure_clause + opt_recursive
[skip]