[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>

[Nikita Pettik <korablev@tarantool.org>]

On Wed, Aug 28, 2019 at 03:17:41PM +0300, Roman Khabibov wrote:
> 
> > On Aug 20, 2019, at 22:41, n.pettik <korablev@tarantool.org> wrote:
> > You slightly misunderstood me. I proposed to allow such queries
> > gracefully handling WITH statement and avoiding references
> > counter incrementation for CTEs.
> 
> commit 202677704c455208d6def08d373776cfae82fdbe
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Mon Jul 29 18:00:34 2019 +0300
> 
>     sql: allow to create view as <WITH> clause
>     
>     Allow views to use CTEs in <WITH> clauses, which can be in any
>     (nested) select after <AS>.

Please, provide decent commit message which would include problem
explanation, chosen solution, some of implementation details.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 92f1d5b22..d3d6770cf 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1721,6 +1721,8 @@ update_view_references(struct Select *select, int update_value,
>  		const char *space_name = sql_src_list_entry_name(list, i);
>  		if (space_name == NULL)
>  			continue;
> +		if (is_cte(select, space_name) == true)
> +			continue;

If function returns boolean value, you don't need to use comparison
to check it:

'if (returns_boolean()) ...' or 'if (! returns_boolean()) ...'

Also, put comment explaining why do you skip reference counter
increment in case of SELECT containts CTE.

> diff --git a/src/box/sql.h b/src/box/sql.h
> index 9ccecf28c..c180e40e1 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select);
>  struct SrcList *
>  sql_select_expand_from_tables(struct Select *select);
>  
> +/**
> + * Check if @a name matches with at least one of CTE names typed
> + * in <WITH> clauses within @a select.
> + *
> + * @param select Select to be checked.

-> Select which may contain CTE.

> + * @retval true Has CTE with @a name.
> + * @retval false Hasn't CTE with @a name.
> +*/
> +bool
> +is_cte(struct Select *select, const char *name);
> +
>  /**
>   * Temporary getter in order to avoid including sqlInt.h
>   * in alter.cc.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index c312f61f1..0ee840b89 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -332,6 +332,25 @@ sql_select_expand_from_tables(struct Select *select)
>  	return walker.u.pSrcList;
>  }
>  
> +bool
> +is_cte(struct Select *select, const char *name)

Such a bad name. Please, get in touch with our naming policy
and come up with better one.

> +{
> +	assert(select != NULL && name != NULL);
> +	struct With *with = select->pWith;
> +	if (with != NULL) {
> +		if (memcmp(name, with->a->zName, strlen(name)) == 0)
> +			return true;
> +	}
> +	struct SrcList *list = select->pSrc;
> +	int item_count = sql_src_list_entry_count(list);
> +	for (int i = 0; i < item_count; ++i) {
> +		if (list->a[i].pSelect != NULL)
> +			if (is_cte(list->a[i].pSelect, name) == true)
> +				return true;
> +	}

It's wrong way of CTE traversal. See sqlTreeViewWith() for correct
implementation. With your approach this example still fails:

create view v as WITH RECURSIVE
  xaxis(x) AS (VALUES(-2.0) UNION ALL SELECT x+0.05 FROM xaxis WHERE x<1.2),
  yaxis(y) AS (VALUES(-1.0) UNION ALL SELECT y+0.1 FROM yaxis WHERE y<1.0),
  m(iter, cx, cy, x, y) AS (
    SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis
    UNION ALL
    SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m 
     WHERE (x*x + y*y) < 4.0 AND iter<28
  ),
  m2(iter, cx, cy) AS (
    SELECT max(iter), cx, cy FROM m GROUP BY cx, cy
  ),
  a(t) AS (
    SELECT group_concat( substr(' .+*#', 1+least(iter/7,4), 1), '') 
    FROM m2 GROUP BY cy
  )
SELECT group_concat(trim(t),x'0a') FROM a;

> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 101f4c3e7..4b695fa6a 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1233,4 +1233,99 @@ test:do_catchsql_test(
>          -- </view-23.8>
>      })
>  
> +-- gh-4149: Check error message for view creation with (nested)
> +-- select with <WITH> clause.

-> gh-4149: make sure that VIEW can be created as CTE.

> +test:do_execsql_test(
> +    "view-24.1",
> +    [[
> +        CREATE TABLE ts (s1 INT PRIMARY KEY);
> +        INSERT INTO ts VALUES (1);
> +    ]], {
> +        -- <view-24.1>
> +        -- </view-24.1>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.2",
> +    [[
> +        CREATE VIEW v AS WITH w(id) AS (
> +            SELECT 1)
> +          SELECT * FROM ts;
> +    ]], {

Please test not only the fact that view can be created, but also that
it is queryable (i.e. SELECT * FROM v is processed without accidents).

> +        -- <view-24.2>
> +        -- </view-24.2>
> +    })
> 

All examples below are almost identical since nCte for them equals to 1.

> +test:do_execsql_test(
> +    "view-24.3",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS WITH RECURSIVE w AS (
> +            SELECT s1 FROM ts
> +            UNION ALL
> +            SELECT s1+1 FROM w WHERE s1 < 4)
> +          SELECT * FROM w;
> +    ]], {
> +        -- <view-24.3>
> +        -- </view-24.3>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.4",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS SELECT * FROM (
> +            WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +              SELECT * FROM w);
> +    ]], {
> +        -- <view-24.4>
> +        -- </view-24.4>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.5",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS SELECT * FROM (
> +            SELECT * FROM (
> +                WITH RECURSIVE w AS (
> +                    SELECT s1 FROM ts
> +                    UNION ALL
> +                    SELECT s1+1 FROM w WHERE s1 < 4)
> +                  SELECT * FROM w));
> +    ]], {
> +        -- <view-24.5>
> +        -- </view-24.5>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.6",
> +    [[
> +        DROP VIEW v;
> +        CREATE VIEW v AS SELECT * FROM
> +            (SELECT 1),
> +            (SELECT 2) JOIN
> +            (WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +                SELECT * FROM w);
> +    ]], {
> +        -- <view-24.6>
> +        -- </view-24.6>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.7",
> +    [[
> +        DROP VIEW v;
> +        DROP TABLE ts;

You don't have to provide clean-up in SQL-tap suite.