[tarantool-patches] Re: [PATCH 1/2] sql: fix collation node duplication in AST
Roman Khabibov
roman.habibov at tarantool.org
Tue Jun 4 17:00:49 MSK 2019
Hello, thanks for the review.
> On Jun 2, 2019, at 8:09 PM, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>
> Hi! Thanks for the fixes! See 2 comments below.
>
> On 28/05/2019 17:10, Roman Khabibov wrote:
>> Hi! Thanks for the review.
>>
>>> I vote for the second as the simplest, and add a test provided by me
>>> above to ensure we will never break it accidentally.
>>>
>>> This commit should consist of the test and a comment in resolveAlias.
>>> And keep this nice ASCII schema.
>>
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 504096e6d..f952d8c04 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -109,6 +109,11 @@ resolveAlias(Parse * pParse, /* Parsing context */
>> return;
>> if (zType[0] != 'G')
>> incrAggFunctionDepth(pDup, nSubquery);
>> + /*
>> + * If there was typed more than one explicit collations in
>> + * query, it will be a sequence of left nodes with the
>> + * collations in a tree.
>> + */
>
> 1. Please, add a comment, that there is nothing special about
> keeping the sequence. Only one collation could be stored, but
> the present solution is simpler.
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 504096e6d..30b9bd9d6 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -109,6 +109,13 @@ resolveAlias(Parse * pParse, /* Parsing context */
return;
if (zType[0] != 'G')
incrAggFunctionDepth(pDup, nSubquery);
+ /*
+ * If there was typed more than one explicit collations in
+ * query, it will be a sequence of left nodes with the
+ * collations in a tree. There is nothing special about
+ * keeping the sequence. Only one collation could be
+ * stored, but the present solution is simpler.
+ */
if (pExpr->op == TK_COLLATE) {
pDup =
sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
>> if (pExpr->op == TK_COLLATE) {
>> pDup =
>> sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
>> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
>> index 0bf54576d..3c5d3053a 100755
>> --- a/test/sql-tap/collation.test.lua
>> +++ b/test/sql-tap/collation.test.lua
>> @@ -529,4 +529,21 @@ test:do_catchsql_test(
>> 'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
>> {1, "Collation 'FOO' does not exist"})
>>
>> +-- gh-3805 Check that collation is not ignored. Must pass.
>
> 2. Of course it must pass. It is the purpose of test. Please,
> drop 'Must pass’.
Removed.
commit c409d222c500ef2b355d0c4061a3931aa6374780
Author: Roman Khabibov <roman.habibov at tarantool.org>
Date: Mon May 6 14:45:51 2019 +0300
test: check that collations isn't ignored in SELECTs
Add test to check that a new collation isn't ignored regardless
of a name of a previous one in the following patterns of quries:
SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode_ci"
Also note: It is disallowed to compare strings with different
collations: ISO/IEC JTC 1/SC 32, Part 2: Foundation, page 531
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 504096e6d..30b9bd9d6 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -109,6 +109,13 @@ resolveAlias(Parse * pParse, /* Parsing context */
return;
if (zType[0] != 'G')
incrAggFunctionDepth(pDup, nSubquery);
+ /*
+ * If there was typed more than one explicit collations in
+ * query, it will be a sequence of left nodes with the
+ * collations in a tree. There is nothing special about
+ * keeping the sequence. Only one collation could be
+ * stored, but the present solution is simpler.
+ */
if (pExpr->op == TK_COLLATE) {
pDup =
sqlExprAddCollateString(pParse, pDup, pExpr->u.zToken);
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index 0bf54576d..9d0076e1d 100755
--- a/test/sql-tap/collation.test.lua
+++ b/test/sql-tap/collation.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(174)
+test:plan(177)
local prefix = "collation-"
@@ -529,4 +529,21 @@ test:do_catchsql_test(
'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
{1, "Collation 'FOO' does not exist"})
+-- gh-3805 Check that collation is not ignored.
+
+test:do_execsql_test(
+ "collation-2.6.0",
+ [[ CREATE TABLE a (id INT PRIMARY KEY, s TEXT) ]],
+ {})
+
+test:do_execsql_test(
+ "collation-2.6.1",
+ [[ INSERT INTO a VALUES (1, 'B'), (2, 'b') ]],
+ {})
+
+test:do_execsql_test(
+ "collation-2.6.2",
+ [[ SELECT s COLLATE "unicode_ci" FROM a ORDER BY s COLLATE "unicode" ]],
+ {"b","B"})
+
test:finish_test()
More information about the Tarantool-patches
mailing list