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 76A26220EE for ; Tue, 26 Jun 2018 09:05:54 -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 DyUzudyuZTWU for ; Tue, 26 Jun 2018 09:05:54 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 27B2C22044 for ; Tue, 26 Jun 2018 09:05:54 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables References: <66d8d4aea6bb4a4e5fb96425b86684c542f6ee60.1529998492.git.kyukhin@tarantool.org> From: Vladislav Shpilevoy Message-ID: <7cfee8b1-d640-f3a8-2e98-92c19f03c16b@tarantool.org> Date: Tue, 26 Jun 2018 16:05:51 +0300 MIME-Version: 1.0 In-Reply-To: <66d8d4aea6bb4a4e5fb96425b86684c542f6ee60.1529998492.git.kyukhin@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 patch! See 9 comments below. On 26/06/2018 10:40, Kirill Yukhin wrote: > Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-3369-lua-select > Issue: https://github.com/tarantool/tarantool/issues/3369 > > 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 > --- > src/box/sql.c | 8 +-- > src/box/sql/build.c | 4 +- > src/box/sql/insert.c | 9 ++- > src/box/sql/select.c | 21 ++++++- > test/sql-tap/lua-tables.test.lua | 117 +++++++++++++++++++++++++++++++++++++++ > 5 files changed, 148 insertions(+), 11 deletions(-) > create mode 100755 test/sql-tap/lua-tables.test.lua > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 368bcd6..dd847aa 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -36,6 +36,7 @@ > #include "coll.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > +#include "box/box.h" > #include "box/coll_id_cache.h" > #include "box/schema.h" > #include "box/session.h" > @@ -4798,7 +4799,25 @@ 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); > + sqlite3LocateTable(pParse, LOCATE_NOERR, pFrom->zName); > + if (pTab == NULL) { > + struct Table *tab = sqlite3DbMallocZero(db, > + sizeof(Table)); > + tab->nTabRef = 1; > + 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); 1. Lets use ER_NO_SUCH_SPACE and diag_set. > + pParse->checkSchema = 1; 2. Useless set. checkSchema is never used to check anything. And I can not imagine how this flag can help us in the parser. Parser does not yield and schema can not be changed during parsing. Schema version is needed in VDBE only and should be checked always before running the program. > + return WRC_Abort; > + } > + struct space *space = space_by_id(space_id); > + tab->def = space->def; > + pFrom->pTab = pTab = tab; > + } > if (pTab == NULL) > return WRC_Abort; 3. This check makes no sense here. If pTab is not NULL after locate, then this check is true. If pTab was not located, then you go into 'if (pTab == NULL)' branch above (line 4803). But in this branch it can become NULL on failed malloc, and you do not check is, so it can lead to crash. 4. Leak, when space_id == BOX_ID_NIL: you did not delete tab. 5. Are you sure, that you can not allocate this pTab on the parser region so as to do not free it later? For example, make it has 2 refs. I am pushing to use region everywhere it is possible. > if (pTab->nTabRef >= 0xffff) { > diff --git a/test/sql-tap/lua-tables.test.lua b/test/sql-tap/lua-tables.test.lua > new file mode 100755 > index 0000000..3096cd7 > --- /dev/null > +++ b/test/sql-tap/lua-tables.test.lua > @@ -0,0 +1,117 @@ > +#!/usr/bin/env tarantool > +test = require("sqltester") > +test:plan(7) > + > +test:do_test( > + "lua-tables-repare-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" 6. This syntax looks weird and unreadable, with "" around each identifier. Can we store all identifiers in lower case? Or exactly upper is the standard? I thought it is implementation detail, it is not? > + ]], > + {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 tables specified"} 7. Why 'no tables'? There is table t1, that does not exist. Why not 'table t1 does not exist'? > +) > + > +-- Extract from tkt3527.test.lua > +test:do_test( > + "lua-tables-prepare-5", > + function() > + format = {{name = "CODE", type = "integer"}, 8. Why here 'integer' but 'scalar' in other places? > + {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-6", > + [[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-7", > + [[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() > 9. What if I use 'indexed by'? It requires index name. Will be it found in struct space?