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 0B6AF21273 for ; Wed, 27 Jun 2018 08:23:51 -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 dXgqvj0R671l for ; Wed, 27 Jun 2018 08:23:50 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 AE73A2147C for ; Wed, 27 Jun 2018 08:23:50 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables References: <66d8d4aea6bb4a4e5fb96425b86684c542f6ee60.1529998492.git.kyukhin@tarantool.org> <7cfee8b1-d640-f3a8-2e98-92c19f03c16b@tarantool.org> <20180626160412.u56zrmkmlczgt5e7@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 27 Jun 2018 15:23:48 +0300 MIME-Version: 1.0 In-Reply-To: <20180626160412.u56zrmkmlczgt5e7@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Kirill Yukhin Cc: tarantool-patches@freelists.org Hello. Thanks for the fixes! See 6 comments below. All the comments except point 6 I have fixed and pushed as a separate commit. Please, look and squash. (I did not run tests, so maybe some of my fixes can break them). > > commit 93d8e00322a6822649d2358eceb3653632eb1d27 > Author: Kirill Yukhin > Date: Tue Jun 26 09:58:17 2018 +0300 > > sql: enable basic SELECTs for Lua created tables > > Update expander of SELECT statement to search for space if > corresponding table wasn't found in table hash. Create dummy > table if so and fill def field only. > > Also fix a leak in VIEW creation. > > Part of #3369 > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 368bcd6..755cb0c 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -4798,9 +4799,34 @@ selectExpander(Walker * pWalker, Select * p) > /* An ordinary table or view name in the FROM clause */ > assert(pFrom->pTab == 0); > pFrom->pTab = pTab = > - sqlite3LocateTable(pParse, 0, pFrom->zName); > - if (pTab == NULL) > - return WRC_Abort; > + sqlite3LocateTable(pParse, LOCATE_NOERR, pFrom->zName); 1. Multiple 80 symbols violation below. > + if (pTab == NULL) { > + const char *t_name = pFrom->zName; > + int space_id = box_space_id_by_name(t_name, > + strlen(t_name)); > + if (space_id == BOX_ID_NIL) { > + sqlite3ErrorMsg(pParse, > + "no such table: %s", > + t_name); > + return WRC_Abort; > + } > + struct space *space = space_by_id(space_id); > + > + if (space->def->field_count <= 0) { > + sqlite3ErrorMsg(pParse, > + "no format for space: %s", > + t_name); > + return WRC_Abort; > + } > + struct Table *tab = sqlite3DbMallocZero(db, > + sizeof(Table)); > + tab->nTabRef = 1; 2. The tab == NULL is useless, if you use tab before the check. > + if (tab == NULL) > + return WRC_Abort; > + tab->nTabRef = 1; 3. Second setting. > + tab->def = space->def; > + pFrom->pTab = pTab = tab; > + } > if (pTab->nTabRef >= 0xffff) { 4. This check makes no sense for just created dummy table. 5. Below the table is referenced again (it is necessary for non-dymmy tables), so dummy tab leaks always: it has 1 ref on creation, 2 refs after ref below and 1 ref on parser finalization. Never 0. > sqlite3ErrorMsg(pParse, > "too many references to \"%s\": max 65535", > diff --git a/test/sql-tap/lua-tables.test.lua b/test/sql-tap/lua-tables.test.lua > new file mode 100755 > index 0000000..5ccb7bd > --- /dev/null > +++ b/test/sql-tap/lua-tables.test.lua > @@ -0,0 +1,122 @@ > +#!/usr/bin/env tarantool > +test = require("sqltester") > +test:plan(8) > + > +test:do_test( > + "lua-tables-prepare-1", > + function() > + format = {} > + format[1] = { name = 'id', type = 'scalar'} > + format[2] = { name = 'f2', type = 'scalar'} > + s = box.schema.create_space('t', {format = format}) > + i = s:create_index('i', {parts={1, 'scalar'}}) > + > + s:replace{1, 4} > + s:replace{2, 2} > + s:replace{3, 3} > + s:replace{4, 3} > + > + s1 = box.schema.create_space('t1') > + s1:create_index('i', {parts={1, 'scalar'}}) > + s1:replace{1, 1} > + end, > + {}) > + > +test:do_execsql_test( > + "lua-tables-2", > + [[SELECT *, count(*) > + FROM "t" as t1, "t" as t2 > + WHERE t1."id" = t2."f2" > + ]], > + {4, 3, 1, 4, 4}) > + > +test:do_execsql_test( > + "lua-tables-3", > + [[create view v as SELECT * FROM "t"; > + select * from v; > + ]], > + {1, 4, 2, 2, 3, 3, 4, 3}) > + > +test:do_catchsql_test( > + "lua-tables-4", > + [[SELECT * from t1]], > + {1, "no such table: T1"} > +) > + > +test:do_catchsql_test( > + "lua-tables-5", > + [[SELECT * from "t1"]], > + {1, "no format for space: t1"} > +) > +-- Extract from tkt3527.test.lua > +test:do_test( > + "lua-tables-prepare-6", > + function() > + format = {{name = "CODE", type = "integer"}, > + {name = "NAME", type = "scalar"}} > + s = box.schema.create_space("ELEMENT", {format = format}) > + s:create_index('pk', {parts = {1, 'scalar'}}) > + s:replace{1, 'Elem1'} > + s:replace{2, 'Elem2'} > + s:replace{3, 'Elem3'} > + s:replace{4, 'Elem4'} > + s:replace{5, 'Elem5'} > + > + format = {{name = "CODEOR", type = "scalar"}, > + {name = "CODE", type = "scalar"}} > + s = box.schema.create_space("ELEMOR", {format = format}) > + s:create_index('pk', {parts = {1, 'scalar', 2, 'scalar'}}) > + s:replace{3, 4} > + s:replace{4, 5} > + > + format = {{name = "CODEAND", type = "scalar"}, > + {name = "CODE", type = "scalar"}, > + {name = "ATTR1", type = "scalar"}, > + {name = "ATTR2", type = "scalar"}, > + {name = "ATTR3", type = "scalar"}} > + s = box.schema.create_space("ELEMAND", {format = format}) > + s:create_index('pk', {parts = {1, "scalar", 2, "scalar"}}) > + s:replace{1, 3, 'a', 'b', 'c'} > + s:replace{1, 2, 'x', 'y', 'z'} > + end, > + {}) > + > +test:do_execsql_test( > + "lua-tables-7", > + [[CREATE VIEW ElemView1 AS > + SELECT > + CAST(Element.Code AS VARCHAR(50)) AS ElemId, > + Element.Code AS ElemCode, > + Element.Name AS ElemName, > + ElemAnd.Code AS InnerCode, > + ElemAnd.Attr1 AS Attr1, > + ElemAnd.Attr2 AS Attr2, > + ElemAnd.Attr3 AS Attr3, > + 0 AS Level, > + 0 AS IsOrElem > + FROM Element JOIN ElemAnd ON ElemAnd.CodeAnd=Element.Code > + WHERE ElemAnd.CodeAnd NOT IN (SELECT CodeOr FROM ElemOr) > + UNION ALL > + SELECT > + CAST(ElemOr.CodeOr AS VARCHAR(50)) AS ElemId, > + Element.Code AS ElemCode, > + Element.Name AS ElemName, > + ElemOr.Code AS InnerCode, > + NULL AS Attr1, > + NULL AS Attr2, > + NULL AS Attr3, > + 0 AS Level, > + 1 AS IsOrElem > + FROM ElemOr JOIN Element ON Element.Code=ElemOr.CodeOr > + ORDER BY ElemId, InnerCode;]], > + {}) > + > +test:do_execsql_test( > + "lua-tables-8", > + [[SELECT * FROM ElemView1]], > + {"1",1,"Elem1",2,"x","y","z",0,0, > + "1",1,"Elem1",3,"a","b","c",0,0, > + "3",3,"Elem3",4,"","","",0,1, > + "4",4,"Elem4",5,"","","",0,1}) > + > +test:finish_test() > 6. So what about INDEXED BY? It leads to a crash? Or to an error? Or what? A test is needed.