From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A555E2B271 for ; Thu, 28 Mar 2019 16:13:08 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XNiFbsbHoP8V for ; Thu, 28 Mar 2019 16:13:08 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 490DC2AFB5 for ; Thu, 28 Mar 2019 16:13:08 -0400 (EDT) Date: Thu, 28 Mar 2019 23:13:05 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v9 7/7] sql: remove box.sql.execute() Message-ID: <20190328201305.GA16354@tarantool.org> References: <40ed2682703f6c62298587dd74d997f71c124411.1553251042.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org 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.