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

[Mergen Imeev]

Hi! Thank you for review. My answers, diff and new patch below.

On Wed, Mar 27, 2019 at 12:48:17AM +0300, Vladislav Shpilevoy wrote:
> 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.
> 
After load_cfg() is executed, this version of box.execute() will
be replaced by a new one, and load_cfg() will no longer be
executed. At the same time, if load_cfg() does not replace this
box.execute() with a new one, Tarantool may hang. Not sure that
metatables can fix this problem. Should I use metatables or it is
enough to fix comment? This issue should be automatically fixed
after feature "load cfg after anything from box is called".

> >  -- 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?
> 
This is something like regular expression. I did this because two
errors appears this test:
"Duplicate key exists ..."
and
"Failed to execute SQL statement: Duplicate key exists ..."

This way I can catch both of them.

> >      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?
> 
Fixed:

diff --git a/test/sql-tap/lua/sqltester.lua b/test/sql-tap/lua/sqltester.lua
index af93baa..4450f5a 100644
--- a/test/sql-tap/lua/sqltester.lua
+++ b/test/sql-tap/lua/sqltester.lua
@@ -416,15 +416,10 @@ test.do_eqp_test = function (self, label, sql, result)
         label,
         function()
             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])
+                result[k] = v:totable()
             end
-            return res
+            return result
         end,
         result)
 end


> > +            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?
> 
I did this because values returned by test:execsql weren't
numbers. They were cdata. Due to this they were sorted not
the way it was expected.

> >          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?
There is no such test, only result-file left. I thought that it
isn't needed anymore.