From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E00B028956 for ; Thu, 29 Aug 2019 13:59:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id VMCBjd5sluwv for ; Thu, 29 Aug 2019 13:59:59 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 2A38F278A1 for ; Thu, 29 Aug 2019 13:59:59 -0400 (EDT) Date: Thu, 29 Aug 2019 20:59:55 +0300 From: Nikita Pettik Subject: [tarantool-patches] Re: [PATCH] sql: add check for absence in Message-ID: <20190829175955.GA52650@tarantool.org> References: <20190802125252.54621-1-roman.habibov@tarantool.org> <6B834CCD-6A77-42D0-8AAD-B1BD2B8F51A6@tarantool.org> <8BD992A5-0A74-4E60-A239-5FDE85783467@tarantool.org> <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> <1B494CAB-704F-4A62-A00D-257388DDB2B8@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1B494CAB-704F-4A62-A00D-257388DDB2B8@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Roman Khabibov Cc: tarantool-patches@freelists.org On Wed, Aug 28, 2019 at 03:17:41PM +0300, Roman Khabibov wrote: > > > On Aug 20, 2019, at 22:41, n.pettik 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 > Date: Mon Jul 29 18:00:34 2019 +0300 > > sql: allow to create view as clause > > Allow views to use CTEs in clauses, which can be in any > (nested) select after . 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 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( > -- > }) > > +-- gh-4149: Check error message for view creation with (nested) > +-- select 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); > + ]], { > + -- > + -- > + }) > + > +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). > + -- > + -- > + }) > 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; > + ]], { > + -- > + -- > + }) > + > +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); > + ]], { > + -- > + -- > + }) > + > +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)); > + ]], { > + -- > + -- > + }) > + > +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); > + ]], { > + -- > + -- > + }) > + > +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.