[tarantool-patches] Re: [PATCH v9 7/7] sql: remove box.sql.execute()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 5 comments below.

> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 6c9a820..5530b2c 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -505,17 +505,14 @@ end
>  box.cfg = locked(load_cfg)
>  
>  --
> --- This makes possible do box.sql.execute without calling box.cfg
> +-- This makes possible do box.execute without calling box.cfg
>  -- manually. The load_cfg call overwrites following table and

1. There are no metatable anymore as I see. At least an explicit.
Do you really call load_cfg on every single box.execute() call?
If you do - please, do not. It is too slow. Use metatables.

>  -- metatable.
>  --
> -box.sql = {}
> -setmetatable(box.sql, {
> -    __index = function(table, index)
> -        load_cfg()
> -        return box.sql[index]
> -    end,
> -})
> +function box.execute(...)
> +    load_cfg()
> +    return box.execute(...)
> +end
>  
>  -- gh-810:
>  -- hack luajit default cpath
> diff --git a/test/sql-tap/gh2140-trans.test.lua b/test/sql-tap/gh2140-trans.test.lua
> index fe978d1..3843c97 100755
> --- a/test/sql-tap/gh2140-trans.test.lua
> +++ b/test/sql-tap/gh2140-trans.test.lua
> @@ -28,8 +28,8 @@ test:do_execsql_test('rollback1_check',
>  for _, verb in ipairs({'ROLLBACK', 'ABORT'}) do
> -    box.sql.execute('DELETE FROM t2')
> -    answer = "Duplicate key exists in unique index 'unique_unnamed_T1_2' in space 'T1'"
> +    box.execute('DELETE FROM t2')
> +    answer = "/Duplicate key exists in unique index 'unique_unnamed_T1_2' in space 'T1'/"

2. Why are '/' added?

>      test:do_catchsql_test('insert1_'..verb,
>                            [[START TRANSACTION;
>                              INSERT INTO t2 VALUES (20, 2, 2);
> diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
> index 63c5d9b..cb77bc4 100644
> --- a/test/sql-tap/lua/sqltester.lua
> +++ b/test/sql-tap/lua/sqltester.lua
> @@ -403,7 +415,16 @@ test.do_eqp_test = function (self, label, sql, result)
>      test:do_test(
>          label,
>          function()
> -            return execsql_one_by_one("EXPLAIN QUERY PLAN "..sql)
> +            local result = execsql_one_by_one("EXPLAIN QUERY PLAN "..sql)
> +            local res = {}
> +            for k,v in pairs(result) do
> +                res[k] = {}
> +                res[k][1] = v[1]
> +                res[k][2] = v[2]
> +                res[k][3] = v[3]
> +                res[k][4] = tostring(v[4])

3. What is happening here?

> +            end
> +            return res
>          end,
>          result)
>  end
> diff --git a/test/sql-tap/orderby9.test.lua b/test/sql-tap/orderby9.test.lua
> index 191c21b..13f9ec6 100755
> --- a/test/sql-tap/orderby9.test.lua
> +++ b/test/sql-tap/orderby9.test.lua
> @@ -41,6 +41,7 @@ test:do_test(
>          -- separately for the result set and the ORDER BY clause, then the output
>          -- order will be random.
>          local l1 = test:execsql("SELECT random() AS y FROM t1 ORDER BY 1;")
> +        for k,_ in pairs(l1) do l1[k] = tonumber(l1[k]) end
>          local l2 = table.deepcopy(l1)
>          table.sort(l1)
>          return test.is_deeply_regex(l1, l2)
> @@ -50,6 +51,7 @@ test:do_test(
>      1.1,
>      function()
>          local l1 = test:execsql("SELECT random() AS y FROM t1 ORDER BY random();")
> +        for k,_ in pairs(l1) do l1[k] = tonumber(l1[k]) end

4. And in these two hunks?

>          local l2 = table.deepcopy(l1)
>          table.sort(l1)
>          return test.is_deeply_regex(l1, l2)
> diff --git a/test/sql/min-on-index.result b/test/sql/min-on-index.result
> deleted file mode 100644
> index 1b2aadf..0000000
> --- a/test/sql/min-on-index.result
> +++ /dev/null

5. Why removed?