<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hello,</p>
<p>thanks for review again<br>
</p>
<div class="moz-cite-prefix">On 24.02.2021 00:25, Vladislav
Shpilevoy wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bc283192-0d10-7920-6317-cee762dd85ec@tarantool.org">
<pre class="moz-quote-pre" wrap="">Hi! Thanks for the patch!
We are almost there.
See 3 comments below.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
+-- <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/5743">https://github.com/tarantool/tarantool/issues/5743</a>
+local is_gh_5743_closed = false
+if is_gh_5743_closed then
+sql("db2", "") -- luacheck: ignore sql
+local sql_exec = nil
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">For me it's not make sense. Replaced push/pop suppression with variable definition.
</pre>
</blockquote>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>Setted undefined variables/functions to nil everywhere instead of
inline luacheck supressions.</p>
<p>There some places in files lua_sql.test.lua, subquery.test.lua
and minmax2.test.lua where I still use luacheck suppressions</p>
<p> because setting variables/functions to nil doesn't work and
broke tests.<br>
</p>
<blockquote type="cite"
cite="mid:bc283192-0d10-7920-6317-cee762dd85ec@tarantool.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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", {}},
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
<p>Well, reverted hunks back and disabled testcase in the test.</p>
<p>I found a source code of original test [1] and it is not clear</p>
<p>why "<span style="color: rgb(68, 68, 68); font-family: monospace; font-size: 14.9333px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">nullvalue</span>"
redefined there.<br>
</p>
<p>1.
<a class="moz-txt-link-freetext" href="https://www.sqlite.org/src/file?ci=trunk&name=test%2Fe_select.test&ln=1504">https://www.sqlite.org/src/file?ci=trunk&name=test%2Fe_select.test&ln=1504</a></p>
<blockquote type="cite"
cite="mid:bc283192-0d10-7920-6317-cee762dd85ec@tarantool.org">
<pre class="moz-quote-pre" wrap="">
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.
</pre>
</blockquote>
Done<br>
<blockquote type="cite"
cite="mid:bc283192-0d10-7920-6317-cee762dd85ec@tarantool.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- {"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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.</pre>
</blockquote>
Done (except files lua_sql.test.lua, subquery.test.lua and
minmax2.test.lua, see above)<br>
<blockquote type="cite"
cite="mid:bc283192-0d10-7920-6317-cee762dd85ec@tarantool.org">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> return X(381, "X!cmd", [=[["concat",[["execsql","SELECT c FROM t3 WHERE b==10"]],["sql_search_count"]]]=])
end, {
-- <index-11.1>
</pre>
</blockquote>
</blockquote>
</body>
</html>