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

[Vladislav Shpilevoy]

Thanks for the fixes!

On 28/03/2019 23:13, Mergen Imeev wrote:
> 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".

Sorry, now I see that in the previous patch box.execute replaces
any other 'box.execute' versions on load_cfg() invocation. It is
ok.

> 
>>> 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.

Apparently looks like a bug: numbers should be compared correctly.
I confirmed that via a simple test case and opened a bug:
https://github.com/tarantool/tarantool/issues/4089

In your test I added a comment:

        -- Big random() numbers are cdata, but cdata numbers can
        -- not be compared nor sorted correctly.