From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Sat, 29 Jun 2019 13:33:49 +0300 From: Mergen Imeev Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] sql: rework SQL errors of type "expected column count" Message-ID: <20190629103348.GA4372@tarantool.org> References: <461ca6b5a31331d36ef7d23872613f0b1c0de9d0.1558176783.git.imeevma@gmail.com> <70E77545-041F-418E-828C-335A449EF4A6@tarantool.org> <20190525111234.GB9859@tarantool.org> <4C0F73A6-4BD5-4D29-891A-57B47374B463@tarantool.org> <20190611084025.GA27968@tarantool.org> <20190625141905.GA29925@tarantool.org> <20190628161526.dystben65fqeruji@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190628161526.dystben65fqeruji@esperanza> To: Vladimir Davydov Cc: "n.pettik" , tarantool-patches@freelists.org List-ID: Thank you! I rewritten original patch a bit: https://github.com/tarantool/tarantool/tree/imeevma/gh-4204-rework-column-count-errors I'm not sure what I did right, since I changed the patch that was already pushed and cherry-picked it. I can easily rewrite it again if necessary. New patch below. On Fri, Jun 28, 2019 at 07:15:26PM +0300, Vladimir Davydov wrote: > On Tue, Jun 25, 2019 at 05:19:05PM +0300, Mergen Imeev wrote: > > From 98589eacdc8caf5a9db366d782338d5c3e551357 Mon Sep 17 00:00:00 2001 > > Date: Sat, 18 May 2019 13:15:58 +0300 > > Subject: [PATCH] sql: allow to use vectors as left value of IN operator > > > > In SQL, it is allowed to use vector expressions, that is, an > > operation that uses vectors as operands. For instance, vector > > comparison: > > SELECT (1,2,3) < (1,2,4); > > > > Accidentally, routines handling IN operator contained a bug: in > > cases where we used a vector as the left value in the IN operator, > > we received an assertion in debug build or a segmentation fault in > > release. This was due to some legacy code in which it was assumed > > that the left value of the IN operator can have only one column in > > case it is a vector. Let's fix this by allowing vectors of the > > other sizes as the left value of the IN operator and providing > > check which verifies that both sides of IN operator have the same > > dimension. > > > > Closes #4204 > > Pushed to master. I failed to backport this to 2.1 - sql-tap/sql-errors > test doesn't pass. Please either backport the patch by yourself or let > me know which commits should be cherry-picked beside this one. New patch: >From 84272dc7eeee704d5bd3c786b464383fa500da02 Mon Sep 17 00:00:00 2001 Date: Sat, 18 May 2019 13:15:58 +0300 Subject: [PATCH] sql: allow to use vectors as left value of IN operator In SQL, it is allowed to use vector expressions, that is, an operation that uses vectors as operands. For instance, vector comparison: SELECT (1,2,3) < (1,2,4); Accidentally, routines handling IN operator contained a bug: in cases where we used a vector as the left value in the IN operator, we received an assertion in debug build or a segmentation fault in release. This was due to some legacy code in which it was assumed that the left value of the IN operator can have only one column in case it is a vector. Let's fix this by allowing vectors of the other sizes as the left value of the IN operator and providing check which verifies that both sides of IN operator have the same dimension. Closes #4204 diff --git a/src/box/errcode.h b/src/box/errcode.h index 3f8cb8e..c9af7dc 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -246,6 +246,7 @@ struct errcode_record { /*191 */_(ER_SQL_PARSER_LIMIT, "%s %d exceeds the limit (%d)") \ /*192 */_(ER_INDEX_DEF_UNSUPPORTED, "%s are prohibited in an index definition") \ /*193 */_(ER_CK_DEF_UNSUPPORTED, "%s are prohibited in a CHECK constraint definition") \ + /*194 */_(ER_SQL_COLUMN_COUNT, "Unequal number of entries in row expression: left side has %u, but right side - %u") \ /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 3ee9a99..5e76a1b 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2968,22 +2968,16 @@ int sqlExprCheckIN(Parse * pParse, Expr * pIn) { int nVector = sqlExprVectorSize(pIn->pLeft); - const char *err; if ((pIn->flags & EP_xIsSelect)) { if (nVector != pIn->x.pSelect->pEList->nExpr) { - err = "sub-select returns %d columns - expected %d"; int expr_count = pIn->x.pSelect->pEList->nExpr; - diag_set(ClientError, ER_SQL_PARSER_GENERIC, - tt_sprintf(err, expr_count, nVector)); + diag_set(ClientError, ER_SQL_COLUMN_COUNT, nVector, + expr_count); pParse->is_aborted = true; return 1; } } else if (nVector != 1) { - assert((pIn->pLeft->flags & EP_xIsSelect) != 0); - int expr_count = pIn->pLeft->x.pSelect->pEList->nExpr; - err = tt_sprintf("sub-select returns %d columns - expected 1", - expr_count); - diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); + diag_set(ClientError, ER_SQL_COLUMN_COUNT, nVector, 1); pParse->is_aborted = true; return 1; } @@ -4169,10 +4163,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) testcase(op == TK_SELECT); if (op == TK_SELECT && (nCol = pExpr->x.pSelect->pEList->nExpr) != 1) { - const char *err = "sub-select returns %d "\ - "columns - expected 1"; - diag_set(ClientError, ER_SQL_PARSER_GENERIC, - tt_sprintf(err, nCol)); + diag_set(ClientError, ER_SQL_COLUMN_COUNT, + nCol, 1); pParse->is_aborted = true; } else { return sqlCodeSubselect(pParse, pExpr, 0); diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index 789e92f..28cc84e 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -795,15 +795,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) nRight = sqlExprVectorSize(pExpr->pRight); } if (nLeft != nRight) { - testcase(pExpr->op == TK_EQ); - testcase(pExpr->op == TK_NE); - testcase(pExpr->op == TK_LT); - testcase(pExpr->op == TK_LE); - testcase(pExpr->op == TK_GT); - testcase(pExpr->op == TK_GE); - testcase(pExpr->op == TK_BETWEEN); - diag_set(ClientError, ER_SQL_PARSER_GENERIC, - "row value misused"); + diag_set(ClientError, ER_SQL_COLUMN_COUNT, + nLeft, nRight); pParse->is_aborted = true; } break; diff --git a/src/box/sql/select.c b/src/box/sql/select.c index b1ec8c7..fc09068 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -3613,12 +3613,10 @@ substExpr(Parse * pParse, /* Report errors here */ assert(pExpr->pLeft == 0 && pExpr->pRight == 0); if (sqlExprIsVector(pCopy)) { assert((pCopy->flags & EP_xIsSelect) != 0); - const char *err = "sub-select returns %d "\ - "columns - expected 1"; int expr_count = pCopy->x.pSelect->pEList->nExpr; - diag_set(ClientError, ER_SQL_PARSER_GENERIC, - tt_sprintf(err, expr_count)); + diag_set(ClientError, ER_SQL_COLUMN_COUNT, + expr_count, 1); pParse->is_aborted = true; } else { pNew = sqlExprDup(db, pCopy, 0); diff --git a/test/box/misc.result b/test/box/misc.result index a1f7a09..ebad5bd 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -522,6 +522,7 @@ t; 191: box.error.SQL_PARSER_LIMIT 192: box.error.INDEX_DEF_UNSUPPORTED 193: box.error.CK_DEF_UNSUPPORTED + 194: box.error.SQL_COLUMN_COUNT ... test_run:cmd("setopt delimiter ''"); --- diff --git a/test/sql-tap/in1.test.lua b/test/sql-tap/in1.test.lua index 835c10d..67e38a3 100755 --- a/test/sql-tap/in1.test.lua +++ b/test/sql-tap/in1.test.lua @@ -594,7 +594,7 @@ test:do_catchsql_test( SELECT b FROM t1 WHERE a NOT IN tb; ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -677,7 +677,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -689,7 +689,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -701,7 +701,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -713,7 +713,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -823,7 +823,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -1058,7 +1058,7 @@ test:do_catchsql_test( SELECT 0 WHERE (SELECT 0,0) OR (0 IN (1,2)); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 2, but right side - 1" -- }) diff --git a/test/sql-tap/select2.test.lua b/test/sql-tap/select2.test.lua index c7f1e5d..74e40f4 100755 --- a/test/sql-tap/select2.test.lua +++ b/test/sql-tap/select2.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(16) +test:plan(32) --!./tcltestrunner.lua -- 2001 September 15 @@ -269,5 +269,166 @@ test:do_execsql_test( -- }) +test:do_catchsql_test( + "select2-5.1", + [[ + SELECT (1,2) IN (1,2,3); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 1" + -- + }) + +test:do_catchsql_test( + "select2-5.2", + [[ + SELECT (1,2) IN (SELECT (1), (2), (3)); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.3", + [[ + SELECT (1,2) IN (VALUES (1,2,3), (2,3,4)); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.4", + [[ + SELECT (1,2) IN (SELECT * from (VALUES (1,2,3), (2,3,4))); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.5", + [[ + SELECT (1) IN (1,2,3); + ]], { + -- + 0, {1} + -- + }) + +test:do_catchsql_test( + "select2-5.6", + [[ + SELECT (1,2,3) IN (SELECT (1), (2), (3)); + ]], { + -- + 0, {1} + -- + }) + +test:do_catchsql_test( + "select2-5.7", + [[ + SELECT (1,2,3) IN (VALUES (1,2,3), (2,3,4)); + ]], { + -- + 0, {1} + -- + }) + +test:do_catchsql_test( + "select2-5.8", + [[ + SELECT (1,2,3) IN (SELECT * from (VALUES (1,2,3), (2,3,4))); + ]], { + -- + 0, {1} + -- + }) + +test:do_catchsql_test( + "select2-5.9", + [[ + SELECT (SELECT * FROM (VALUES (1,2))) IN (1,2,3); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 1" + -- + }) + +test:do_catchsql_test( + "select2-5.10", + [[ + SELECT (SELECT * FROM (VALUES (1,2))) IN (SELECT (1), (2), (3)); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.11", + [[ + SELECT (SELECT * FROM (VALUES (1,2))) IN (VALUES (1,2,3), (2,3,4)); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.12", + [[ + SELECT (SELECT * FROM (VALUES (1,2))) IN (SELECT * from (VALUES (1,2,3), (2,3,4))); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.13", + [[ + SELECT (1,2,3) = (1,2,3); + ]], { + -- + 0, {1} + -- + }) + +test:do_catchsql_test( + "select2-5.14", + [[ + SELECT (1,2) = (1,2,3); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.15", + [[ + SELECT (SELECT (1), (2)) = (1,2,3); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + +test:do_catchsql_test( + "select2-5.16", + [[ + SELECT (VALUES (1,2)) = (1,2,3); + ]], { + -- + 1, "Unequal number of entries in row expression: left side has 2, but right side - 3" + -- + }) + + test:finish_test() diff --git a/test/sql-tap/select7.test.lua b/test/sql-tap/select7.test.lua index f2a071d..3b36d57 100755 --- a/test/sql-tap/select7.test.lua +++ b/test/sql-tap/select7.test.lua @@ -130,7 +130,7 @@ test:do_catchsql_test( SELECT 5 IN (SELECT a,b FROM t2); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -140,7 +140,7 @@ test:do_catchsql_test( SELECT 5 IN (SELECT * FROM t2); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -150,7 +150,7 @@ test:do_catchsql_test( SELECT 5 IN (SELECT a,b FROM t2 UNION SELECT b,a FROM t2); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) @@ -160,7 +160,7 @@ test:do_catchsql_test( SELECT 5 IN (SELECT * FROM t2 UNION SELECT * FROM t2); ]], { -- - 1, "sub-select returns 2 columns - expected 1" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- }) diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua index 4e173b6..9f7853e 100755 --- a/test/sql-tap/sql-errors.test.lua +++ b/test/sql-tap/sql-errors.test.lua @@ -462,7 +462,7 @@ test:do_catchsql_test( SELECT (1,2,3) == (1,2,3,4); ]], { -- - 1,"row value misused" + 1,"Unequal number of entries in row expression: left side has 3, but right side - 4" -- }) diff --git a/test/sql-tap/subselect.test.lua b/test/sql-tap/subselect.test.lua index 5b71390..2cbceb8 100755 --- a/test/sql-tap/subselect.test.lua +++ b/test/sql-tap/subselect.test.lua @@ -49,7 +49,7 @@ test:do_catchsql_test( SELECT * FROM t1 WHERE a = (SELECT * FROM t1) ]], { -- - 1, "row value misused" + 1, "Unequal number of entries in row expression: left side has 1, but right side - 2" -- })