Hi Diwas, On Wed, Jun 24, 2015 at 07:00:48PM +0530, Diwas Joshi wrote:

hii, please find a patch for MDEV-8343 <https://mariadb.atlassian.net/browse/MDEV-8343> attached to this mail. I have also included a test case in the patch.

Please find my comments below. "Coding style" refers to the MySQL/MariaDB coding style. Its original version is located here https://dev.mysql.com/doc/internals/en/general-development-guidelines.html but in general, you should make your code look like the rest of the code does, and provide adequate documentation. Please also use better subjects for your emails. 'GSoc2015' can be applied to most of emails you send. Something like 'GSoC2015: MDEV-8342 patch for review' should have been used. Please note this when emailing next time. I also get a crash with this patch about which I will email separately.

diff --git a/mysql-test/r/sp-error.result b/mysql-test/r/sp-error.result index 4373925..58be47e 100644 --- a/mysql-test/r/sp-error.result +++ b/mysql-test/r/sp-error.result @@ -2866,3 +2866,9 @@ SELECT @msg; DROP FUNCTION f1; DROP FUNCTION f2; DROP TABLE t1; +CREATE FUNCTION f1(a INT, b VARCHAR(11)) +RETURNS TABLE t1(id INT, name VARCHAR(11)) +BEGIN +INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; +END| +ERROR 42000: This version of MariaDB doesn't yet support '%s' near 'Table functions' at line 3 diff --git a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test index c1a21c8..580f2b4 100644 --- a/mysql-test/t/sp-error.test +++ b/mysql-test/t/sp-error.test @@ -3845,3 +3845,14 @@ SELECT @msg; DROP FUNCTION f1; DROP FUNCTION f2; DROP TABLE t1; + + +delimiter |; +--error 1064 +CREATE FUNCTION f1(a INT, b VARCHAR(11)) +RETURNS TABLE t1(id INT, name VARCHAR(11)) +BEGIN +INSERT INTO t1 SELECT id, name FROM t2 WHERE id = a; +END| + +DELIMITER ;| diff --git a/sql/sp.h b/sql/sp.h index eb3291f..149077f 100644 --- a/sql/sp.h +++ b/sql/sp.h @@ -45,7 +45,8 @@ enum stored_procedure_type TYPE_ENUM_FUNCTION=1, TYPE_ENUM_PROCEDURE=2, TYPE_ENUM_TRIGGER=3, - TYPE_ENUM_PROXY=4 + TYPE_ENUM_PROXY=4, + TYPE_ENUM_TABLE=5 };

/* Tells what SP_DEFAULT_ACCESS should be mapped to */ diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 8efeeab..f705930 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2360,6 +2360,34 @@ void sp_head::recursion_level_error(THD *thd) return FALSE; }

Please add a function comment so that one can undetstand what the function does. The comment should be in this kind-of-like doxygen form: /* @brief <One-line description of what the function does> @detail <detailed description of what the function does. Write this only if you have something to say> @return possible_ret_value Description possible_ret_value Description */

+bool +sp_head::fill_resultset_definition(THD *thd, List<Create_field> *create_list) +{ + List_iterator_fast<Create_field> it(*create_list); + Create_field *field; + while ((field= it++)) + { + Create_field *new_field = field->clone(thd->mem_root); + switch (new_field->sql_type) { + case MYSQL_TYPE_DATE: + case MYSQL_TYPE_NEWDATE: + case MYSQL_TYPE_TIME: + case MYSQL_TYPE_DATETIME: + case MYSQL_TYPE_TIMESTAMP: + new_field->charset= &my_charset_bin; + default: break; + } + if (!new_field->charset) + new_field->charset= default_charset_info; + if (!new_field->charset) + new_field->charset= system_charset_info; + if (new_field->interval_list.elements) + new_field->interval= create_typelib(thd->mem_root, new_field, &new_field->interval_list); Too long line. Coding style says 80 cols max.

+ sp_prepare_create_field(thd, new_field); + if(m_cols_list.push_back(new_field, thd->mem_root)) Coding style says there must be a space after 'if'.

+ return TRUE; + }

What does the function return when the execution reaches here?

+}

int sp_head::new_cont_backpatch(sp_instr_opt_meta *i) diff --git a/sql/sp_head.h b/sql/sp_head.h index dbdb957..4b2fe31 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -182,6 +182,7 @@ class sp_head :private Query_arena uint m_flags; // Boolean attributes of a stored routine

Create_field m_return_field_def; /**< This is used for FUNCTIONs only. */ + List<Create_field> m_cols_list; /**< Used for TABLE FUNCTIONs only. */

const char *m_tmp_query; ///< Temporary pointer to sub query string st_sp_chistics *m_chistics; @@ -196,6 +197,7 @@ class sp_head :private Query_arena LEX_STRING m_defstr; LEX_STRING m_definer_user; LEX_STRING m_definer_host; + LEX_STRING m_table_alias; Please add a comment describing this member.

/** Is this routine being executed? @@ -420,6 +422,7 @@ class sp_head :private Query_arena bool fill_field_definition(THD *thd, LEX *lex, enum enum_field_types field_type, Create_field *field_def); + bool fill_resultset_definition(THD *thd, List<Create_field> *create_list);

void set_info(longlong created, longlong modified, st_sp_chistics *chistics, ulonglong sql_mode); diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index cf933e0b..d3d905b 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1731,7 +1731,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize); %type <string> text_string hex_or_bin_String opt_gconcat_separator

-%type <field_type> type_with_opt_collate int_type real_type field_type +%type <field_type> type_with_opt_collate sf_type_with_opt_collate int_type real_type field_type

%type <geom_type> spatial_type

@@ -6644,7 +6644,25 @@ type_with_opt_collate: } ;

- +sf_type_with_opt_collate: + type_with_opt_collate + { + $$ = $1; + } + | TABLE_SYM ident '(' field_list ')' + { + Lex->sphead->m_table_alias= $2; + // table functions, we need to store the return types + Lex->sphead->m_type= TYPE_ENUM_TABLE; + if(Lex->sphead->fill_resultset_definition(thd, &Lex->alter_info.create_list)) + MYSQL_YYABORT;

coding style: " if (...)"

+ if (Lex->sphead->m_cols_list.is_empty()) + { + my_parse_error(ER(ER_SYNTAX_ERROR)); + MYSQL_YYABORT; + } + } + ; now_or_signed_literal: NOW_SYM opt_default_time_precision { @@ -16283,11 +16301,15 @@ sf_tail: lex->init_last_field(&lex->sphead->m_return_field_def, NULL, thd->variables.collation_database); } - type_with_opt_collate /* $11 */ + sf_type_with_opt_collate /* $11 */ { /* $12 */ - if (Lex->sphead->fill_field_definition(thd, Lex, $11, - Lex->last_field)) - MYSQL_YYABORT; + if (!(Lex->sphead->m_type== TYPE_ENUM_TABLE)) + { + Lex->sphead->m_type = TYPE_ENUM_FUNCTION; + if (Lex->sphead->fill_field_definition(thd, Lex, $11, + Lex->last_field)) + MYSQL_YYABORT; + } } sp_c_chistics /* $13 */ { /* $14 */ @@ -16295,6 +16317,12 @@ sf_tail: Lex_input_stream *lip= YYLIP;

lex->sphead->set_body_start(thd, lip->get_cpp_tok_start()); + + if (Lex->sphead->m_type== TYPE_ENUM_TABLE) //to be removed later. + { + my_parse_error(ER(ER_NOT_SUPPORTED_YET), "Table functions"); + MYSQL_YYABORT; + } } sp_proc_stmt /* $15 */ { @@ -16306,7 +16334,7 @@ sf_tail:

lex->sql_command= SQLCOM_CREATE_SPFUNCTION; sp->set_stmt_end(thd); - if (!(sp->m_flags & sp_head::HAS_RETURN)) + if (!(sp->m_flags & sp_head::HAS_RETURN) && !(Lex->sphead->m_type== TYPE_ENUM_TABLE)) { my_error(ER_SP_NORETURN, MYF(0), sp->m_qname.str); MYSQL_YYABORT;

-- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog