[tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables
Kirill Yukhin
kyukhin at tarantool.org
Tue Jun 26 19:04:12 MSK 2018
Hello Vlad,
Thanks for review. My answers inlined.
Updated patch in the bottom.
On 26 июн 16:05, Vladislav Shpilevoy wrote:
> > 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
> > @@ -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.
I'll defer this change until error reporting fix will be comitted to 2.0.
> > + 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.
I'd rather removed it completely from SQL source base. But OK, removed this one.
> > + 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.
Removed.
> 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.
This is not straightforward: right now Select destruction is common function which
assumes that all entities were malloc()-ed. So, I'll defer such changes until struct
Table is completely vanished.
> > --- /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?
We've talked about dozen times and agreed to store values uppercased.
> > + ]],
> > + {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'?
Fixed error reporting.
> > +-- 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?
This is yet another check. Integer primary key has this type.
> > +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?
No, it won't. Since we have no reference to index_def in struct Index,
so I cannpt create a dummy struct Index. Will do that when corresponding
patch hit 2.0.
--
Regards, Kirill Yukhin
commit 93d8e00322a6822649d2358eceb3653632eb1d27
Author: Kirill Yukhin <kyukhin at tarantool.org>
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.c b/src/box/sql.c
index 1135315..6e5c1b3 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1740,14 +1740,14 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
}
struct space_def *
-sql_ephemeral_space_def_new(Parse *parser, const char *name)
+sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
{
struct space_def *def = NULL;
- struct region *region = &fiber()->gc;
size_t name_len = name != NULL ? strlen(name) : 0;
uint32_t dummy;
- size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy);
- def = (struct space_def *)region_alloc(region, size);
+ size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy,
+ &dummy);
+ def = (struct space_def *)region_alloc(&parser->region, size);
if (def == NULL) {
diag_set(OutOfMemory, size, "region_alloc",
"sql_ephemeral_space_def_new");
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0da7d80..748fb51 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1970,6 +1970,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
struct Select *select, bool if_exists)
{
struct sqlite3 *db = parse_context->db;
+ struct Table *sel_tab = NULL;
if (parse_context->nVar > 0) {
sqlite3ErrorMsg(parse_context,
"parameters are not allowed in views");
@@ -1979,7 +1980,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
struct Table *p = parse_context->pNewTable;
if (p == NULL || parse_context->nErr != 0)
goto create_view_fail;
- struct Table *sel_tab = sqlite3ResultSetOfSelect(parse_context, select);
+ sel_tab = sqlite3ResultSetOfSelect(parse_context, select);
if (sel_tab == NULL)
goto create_view_fail;
if (aliases != NULL) {
@@ -2032,6 +2033,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
sqlite3EndTable(parse_context, 0, &end, 0);
create_view_fail:
+ sqlite3DbFree(db, sel_tab);
sql_expr_list_delete(db, aliases);
sql_select_delete(db, select);
return;
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index a43f7b5..6a8cc69 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -51,11 +51,10 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */
Vdbe *v;
v = sqlite3GetVdbe(pParse);
assert(opcode == OP_OpenWrite || opcode == OP_OpenRead);
- Index *pPk = sqlite3PrimaryKeyIndex(pTab);
- assert(pPk != 0);
- assert(pPk->tnum == pTab->tnum);
- struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
- vdbe_emit_open_cursor(pParse, iCur, pPk->tnum, space);
+
+ struct space *space = space_by_id(pTab->def->id);
+ assert(space->index_count > 0);
+ vdbe_emit_open_cursor(pParse, iCur, 0, space);
VdbeComment((v, "%s", pTab->def->name));
}
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
@@ -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,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);
+ 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;
+ if (tab == NULL)
+ return WRC_Abort;
+ tab->nTabRef = 1;
+ tab->def = space->def;
+ pFrom->pTab = pTab = tab;
+ }
if (pTab->nTabRef >= 0xffff) {
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()
More information about the Tarantool-patches
mailing list