Hello,

thanks for review again

On 24.02.2021 00:25, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> We are almost there.
>
> See 3 comments below.
>
>> diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
>> index 654629bf7..f375fea0f 100755
>> --- a/test/sql-tap/badutf1.test.lua
>> +++ b/test/sql-tap/badutf1.test.lua
>> @@ -93,8 +93,12 @@ test:do_test(
>>       })
>>   
>>   -- commented as it uses utf16
>> -if 0>0 then
>> -sql("db2", "")
>> +-- testcases are broken
>> +-- https://github.com/tarantool/tarantool/issues/5743
>> +local is_gh_5743_closed = false
>> +if is_gh_5743_closed then
>> +sql("db2", "") -- luacheck: ignore sql
>> +local sql_exec = nil
>>> For me it's not make sense. Replaced push/pop suppression with variable definition.
> 1. Lets at least be consistent and either use `-- luacheck` comments or
> `= nil` hack. Not both, especially not on 2 adjacent lines. It simply
> raises unnecessary questions why couldn't it be done in one way.

Setted undefined variables/functions to nil everywhere instead of inline 
luacheck supressions.

There some places in files lua_sql.test.lua, subquery.test.lua and 
minmax2.test.lua where I still use luacheck suppressions

because setting variables/functions to nil doesn't work and broke tests.

>
>>   test:do_test(
>>       "badutf-1.10",
>>       function()> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
>> index 2a189582d..9ef5e7925 100755
>> --- a/test/sql-tap/e_select1.test.lua
>> +++ b/test/sql-tap/e_select1.test.lua
>> @@ -1537,32 +1561,26 @@ test:do_select_tests(
>>   -- considered equal to other NULL values and distinct from all non-NULL
>>   -- values.
>>   --
>> --- MUST_WORK_TEST
>> -if 0>0 then
>> -db("nullvalue", "null")
>>   test:do_select_tests(
>>       "e_select-7.9",
>>       {
>> -        {"1", "SELECT NULL UNION ALL SELECT NULL", {null, null}},
>> -        {"2", "SELECT NULL UNION     SELECT NULL", {null}},
>> -        {"3", "SELECT NULL INTERSECT SELECT NULL", {null}},
>> +        {"1", "SELECT NULL UNION ALL SELECT NULL", {"", ""}},
>> +        {"2", "SELECT NULL UNION     SELECT NULL", {""}},
>> +        {"3", "SELECT NULL INTERSECT SELECT NULL", {""}},
>>           {"4", "SELECT NULL EXCEPT    SELECT NULL", {}},
> 2. But why? What was the effect of `db("nullvalue", "null")` in the
> original tests? If you don't know then maybe it is worth leaving it
> as is + adding `local null = box.NULL` or `local null = nil` so at
> least we could preserve the test body unchanged for future
> investigations.

Well, reverted hunks back and disabled testcase in the test.

I found a source code of original test [1] and it is not clear

why "nullvalue" redefined there.

1. 
https://www.sqlite.org/src/file?ci=trunk&name=test%2Fe_select.test&ln=1504

> To me it is not clear why null can be "". Empty string is not the
> same as NULL.
>
> The same for the other `db("nullvalue", "null")` removals where
> `null` was used for anything. If there are such other places.
Done
>> -        {"5", "SELECT NULL UNION ALL SELECT 'ab'", {null, "ab"}},
>> -        {"6", "SELECT NULL UNION     SELECT 'ab'", {null, "ab"}},
>> +        {"5", "SELECT NULL UNION ALL SELECT 'ab'", {"", "ab"}},
>> +        {"6", "SELECT NULL UNION     SELECT 'ab'", {"", "ab"}},
>>           {"7", "SELECT NULL INTERSECT SELECT 'ab'", {}},
>> -        {"8", "SELECT NULL EXCEPT    SELECT 'ab'", {null}},
>> -        {"9", "SELECT NULL UNION ALL SELECT 0", {null, 0}},
>> -        {"10", "SELECT NULL UNION     SELECT 0", {null, 0}},
>> +        {"8", "SELECT NULL EXCEPT    SELECT 'ab'", {""}},
>> +        {"9", "SELECT NULL UNION ALL SELECT 0", {"", 0}},
>> +        {"10", "SELECT NULL UNION     SELECT 0", {"", 0}},
>>           {"11", "SELECT NULL INTERSECT SELECT 0", {}},
>> -        {"12", "SELECT NULL EXCEPT    SELECT 0", {null}},
>> -        {"13", "SELECT c FROM q1 UNION ALL SELECT g FROM q3", {null, -42.47, "null", 2, 2}},
>> -        {"14", "SELECT c FROM q1 UNION     SELECT g FROM q3", {null, -42.47, 2}},
>> +        {"12", "SELECT NULL EXCEPT    SELECT 0", {""}},
>> +        {"13", "SELECT c FROM q1 UNION ALL SELECT g FROM q3", {"", -42.47, "", 2, 2}},
>> +        {"14", "SELECT c FROM q1 UNION     SELECT g FROM q3", {"", -42.47, 2}},
>>           {"15", "SELECT c FROM q1 INTERSECT SELECT g FROM q3", {}},
>> -        {"16", "SELECT c FROM q1 EXCEPT    SELECT g FROM q3", {null, -42.47}},
>> +        {"16", "SELECT c FROM q1 EXCEPT    SELECT g FROM q3", {"", -42.47}},
>>       })
>> -
>> -db("nullvalue", "")
>> -end
>> diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
>> index ea25727a4..574faafd6 100755
>> --- a/test/sql-tap/index1.test.lua
>> +++ b/test/sql-tap/index1.test.lua
>> @@ -585,7 +585,7 @@ test:do_test(
>>           for i = 1, 50, 1 do
>>               test:execsql(string.format("INSERT INTO t3 VALUES('x%sx',%s,0.%s)", i, i, i))
>>           end
>> -        local sql_search_count = 0
>> +        -- luacheck: ignore X
> 3. Lets be consistent and not jump from one method to another.
> We either use luacheck comments or `local X = nil` hacks. The same
> for other similar inconsistencies in this and the rest of the patches.
Done (except files lua_sql.test.lua, subquery.test.lua and 
minmax2.test.lua, see above)
>
>>           return X(381, "X!cmd", [=[["concat",[["execsql","SELECT c FROM t3 WHERE b==10"]],["sql_search_count"]]]=])
>>       end, {
>>           -- <index-11.1>