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 <sa...@mariadb.com>
> committer: Oleksandr Byelkin <sa...@mariadb.com>
> 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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
