maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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