* [tarantool-patches] [PATCH] sql: enable basic SELECTs for Lua created tables
@ 2018-06-26 7:40 Kirill Yukhin
2018-06-26 13:05 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Kirill Yukhin @ 2018-06-26 7:40 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin
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.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..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);
+ pParse->checkSchema = 1;
+ 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;
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"
+ ]],
+ {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"}
+)
+
+-- Extract from tkt3527.test.lua
+test:do_test(
+ "lua-tables-prepare-5",
+ 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-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()
--
2.16.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables
2018-06-26 7:40 [tarantool-patches] [PATCH] sql: enable basic SELECTs for Lua created tables Kirill Yukhin
@ 2018-06-26 13:05 ` Vladislav Shpilevoy
2018-06-26 16:04 ` Kirill Yukhin
0 siblings, 1 reply; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-26 13:05 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables
2018-06-26 13:05 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-26 16:04 ` Kirill Yukhin
2018-06-27 12:23 ` Vladislav Shpilevoy
0 siblings, 1 reply; 4+ messages in thread
From: Kirill Yukhin @ 2018-06-26 16:04 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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@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()
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables
2018-06-26 16:04 ` Kirill Yukhin
@ 2018-06-27 12:23 ` Vladislav Shpilevoy
0 siblings, 0 replies; 4+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-27 12:23 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
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 <kyukhin@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/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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-27 12:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 7:40 [tarantool-patches] [PATCH] sql: enable basic SELECTs for Lua created tables Kirill Yukhin
2018-06-26 13:05 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-26 16:04 ` Kirill Yukhin
2018-06-27 12:23 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox