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

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