maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sanja!

_some_ comments are below.

The main thing, I still don't understand your changes in sql_select.cc.

Why did you create separate counters and code paths with if() for CYCLE?
I'd think it could be just a generalization of DISTINCT

On Feb 20, Oleksandr Byelkin wrote:
> revision-id: 4c263b6d30b (mariadb-10.5.0-92-g4c263b6d30b)
> parent(s): 6f2e2285291
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2020-01-27 21:56:02 +0100
> message:
> 
> MDEV-20632: Recursive CTE cycle detection using CYCLE clause
> 
> Added CYCLE clause to recursive CTE.
>
> diff --git a/sql/item.h b/sql/item.h
> index c1f68a4f942..2cbaf880e00 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -622,6 +622,13 @@ class st_select_lex_unit;
>  class Item_func_not;
>  class Item_splocal;
>  
> +/* Item::common_flags */
> +/* Indicates that name of this Item autogenerated or set by user */
> +#define IS_AUTO_GENETATED_NAME 1

typo ^^^

> +/* Inticates that this item is in CYCLE clause of WITH */

typo ^^^

> +#define IS_IN_WITH_CYCLE       2
> +
> +
>  /**
>    String_copier that sends Item specific warnings.
>  */
> diff --git a/sql/sql_cte.cc b/sql/sql_cte.cc
> index 5b3d3108da5..971844120bc 100644
> --- a/sql/sql_cte.cc
> +++ b/sql/sql_cte.cc
> @@ -982,6 +982,38 @@ With_element::rename_columns_of_derived_unit(THD *thd,

are you sure what you're doing below can still be called
"rename_columns_of_derived_unit" ?

>    else
>      make_valid_column_names(thd, select->item_list);
>  
> +  if (cycle_list)
> +  {
> +    List_iterator_fast<Item> it(select->item_list);
> +    List_iterator_fast<LEX_CSTRING> nm(*cycle_list);
> +    List_iterator_fast<LEX_CSTRING> nm_check(*cycle_list);
> +    DBUG_ASSERT(cycle_list->elements != 0);
> +    while (LEX_CSTRING *name= nm++)
> +    {
> +      Item *item;
> +      // unique check
> +      LEX_CSTRING *check;
> +      nm_check.rewind();
> +      while ((check= nm_check++) && check != name)
> +      {
> +        if (check->length == name->length &&
> +            strncmp(check->str, name->str, name->length) == 0)
> +        {
> +          my_error(ER_DUP_FIELDNAME, MYF(0), check->str);
> +          return true;
> +        }
> +      }
> +      while ((item= it++) &&
> +             (item->name.length != name->length ||
> +              strncmp(item->name.str, name->str, name->length) != 0));
> +      if (item == NULL)
> +      {
> +        my_error(ER_BAD_FIELD_ERROR, MYF(0), name->str, "CYCLE clause");
> +        return true;
> +      }

this pattern is used so often that we might want to have
a helper, like, List_iterator_fast<>::find()

not in this commit, though.

> +      item->common_flags|= IS_IN_WITH_CYCLE;
> +    }
> +  }
>    unit->columns_are_renamed= true;
>  
>    return false;
> @@ -1425,6 +1457,21 @@ void With_clause::print(String *str, enum_query_type query_type)
>  }
>  
>  
> +static void list_strlex_print(String *str, List<LEX_CSTRING> *list)
> +{
> +  List_iterator_fast<LEX_CSTRING> li(*list);
> +  bool first= TRUE;
> +  while(LEX_CSTRING *col_name= li++)
> +  {
> +    if (first)
> +      first= FALSE;
> +    else
> +      str->append(',');
> +    str->append(col_name);

should be "append_identifier", shouldn't it?
add a test where a column name is a reserved word.

> +  }
> +}
> +
> +
>  /**
>     @brief
>       Print this with element
> @@ -1444,22 +1491,20 @@ void With_element::print(String *str, enum_query_type query_type)
>    {
>      List_iterator_fast<LEX_CSTRING> li(column_list);
>      str->append('(');
> -    for (LEX_CSTRING *col_name= li++; ; )
> -    {
> -      str->append(col_name);
> -      col_name= li++;
> -      if (!col_name)
> -      {
> -        str->append(')');
> -        break;
> -      }
> -      str->append(',');
> -    }
> +    list_strlex_print(str, &column_list);

any other code that could make use of your list_strlex_print ?

> +    str->append(')');
>    }
> -  str->append(STRING_WITH_LEN(" as "));
> -  str->append('(');
> +  str->append(STRING_WITH_LEN(" as ("));
>    spec->print(str, query_type);
>    str->append(')');
> +
> +  if (cycle_list)
> +  {
> +    DBUG_ASSERT(cycle_list->elements != 0);
> +    str->append(STRING_WITH_LEN(" CYCLE ("));
> +    list_strlex_print(str, cycle_list);
> +    str->append(") ");
> +  }
>  }
>  
>  
> diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> index c115d9352aa..0d60ab8579f 100644
> --- a/sql/sql_yacc.yy
> +++ b/sql/sql_yacc.yy
> @@ -14704,9 +14704,30 @@ with_list_element:
>              if (elem->set_unparsed_spec(thd, spec_start, $6.pos(),
>                                          spec_start - query_start))
>                MYSQL_YYABORT;
> +            if ($7)
> +            {
> +              elem->set_cycle_list($7);
> +            }
>  	  }
>  	;
>  
> +opt_cycle:
> +         /* empty */
> +         { $$= NULL; }
> +         |
> +         CYCLE_SYM
> +         {
> +           if (!Lex->curr_with_clause->with_recursive)
> +           {
> +             thd->parse_error(ER_SYNTAX_ERROR, $1.pos());
> +           }
> +         }
> +         '(' with_column_list ')'

Where did you see that the standard requires parentheses here?

> +         {
> +           $$= $4;
> +         }
> +         ;
> +
>  
>  opt_with_column_list:
>            /* empty */

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx