[tarantool-patches] Re: [PATCH] sql: enable basic SELECTs for Lua created tables

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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?