[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>
Nikita Pettik
korablev at tarantool.org
Thu Aug 29 20:59:55 MSK 2019
On Wed, Aug 28, 2019 at 03:17:41PM +0300, Roman Khabibov wrote:
>
> > On Aug 20, 2019, at 22:41, n.pettik <korablev at 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 at 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.
More information about the Tarantool-patches
mailing list