From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0722030FD6 for ; Tue, 25 Jun 2019 10:19:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qR0z6ROAKyAF for ; Tue, 25 Jun 2019 10:19:12 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9A05130FBF for ; Tue, 25 Jun 2019 10:19:12 -0400 (EDT) Date: Tue, 25 Jun 2019 17:19:05 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: rework SQL errors of type "expected column count" Message-ID: <20190625141905.GA29925@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> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190611084025.GA27968@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" Cc: tarantool-patches@freelists.org Fixed commit-message. On Tue, Jun 11, 2019 at 11:40:25AM +0300, Mergen Imeev wrote: > On Thu, Jun 06, 2019 at 10:22:49PM +0300, n.pettik wrote: > > > > >>> /* > > >>> * !IMPORTANT! Please follow instructions at start of the file > > >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > > >>> index ba7eea5..8cf4d2e 100644 > > >>> --- a/src/box/sql/expr.c > > >>> +++ b/src/box/sql/expr.c > > >>> @@ -2958,28 +2958,22 @@ sqlCodeSubselect(Parse * pParse, /* Parsing context */ > > >>> * Expr pIn is an IN(...) expression. This function checks that the > > >>> * sub-select on the RHS of the IN() operator has the same number of > > >>> * columns as the vector on the LHS. Or, if the RHS of the IN() is not > > >>> - * a sub-query, that the LHS is a vector of size 1. > > >>> + * a sub-query, that the LHS must be a vector of size 1. > > >> > > >> What is the point of changing this comment? > > >> > > > In SQlite it wasn't possible to write something like > > > "SELECT (1,2) in ..." so if length of the vector were more > > > than 1 it means it was received using subselect. It isn't > > > so in our case since we can work with such statements. > > > To emphasize this, I changed the comment. > > > > ‘… is a vector…’ -> ‘… must be a vector…' > > > > In context of comment they mean the same -> > > meaningless change. > > > Fixed. > > > > New patch: > > > > > > From 32b665f9ad9dacc16a492c55396dd843f9a355e9 Mon Sep 17 00:00:00 2001 > > > Date: Sat, 18 May 2019 13:15:58 +0300 > > > Subject: [PATCH] sql: rework SQL errors of type "expected column count" > > > > > > Before this patch, errors of type "expected column count" were > > > divided into several different errors, and they all had an > > > ER_SQL_PARSER_GENERIC error code. This patch creates a new error > > > code for these errors. > > > > > > In addition, at some point it became possible to use vectors as > > > the left value in the IN operator. Since this was previously > > > impossible, it led to a segmentation error. > > > > This doesn’t seem to be decent description, at least to me. > > Please, provide description of problem in details: since it > > was assertion fault I guess it deserves to be explained. > > > > Let me help you: > > > > In SQL it is allowed to use vector expressions, i.e. > > * brief description of vector expressions *. > > For instance, * example of using vector expressions * > > Accidentally, routines handling IN operator and vector > > expressions contained bug. * Bug description * > > Let’s fix it in the following way. * Fix description * > > > > > Fixed: > > 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 or a segmentation error. > Let's fix this by allowing vectors as the left value in the IN > operator and using additional checks. > > Closes #4204 > New patch: >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 diff --git a/src/box/errcode.h b/src/box/errcode.h index 55299b7..be8dab2 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -249,6 +249,7 @@ struct errcode_record { /*194 */_(ER_MULTIKEY_INDEX_MISMATCH, "Field %s is used as multikey in one index and as single key in another") \ /*195 */_(ER_CREATE_CK_CONSTRAINT, "Failed to create check constraint '%s': %s") \ /*196 */_(ER_CK_CONSTRAINT_FAILED, "Check constraint failed '%s': %s") \ + /*197 */_(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 898d16c..d7104d8 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2945,22 +2945,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; } @@ -4111,10 +4105,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 fdf3703..0b90edd 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -776,15 +776,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 a257e72..7c8da25 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -3582,12 +3582,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 ec2c4fa..6e1c70d 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -524,6 +524,7 @@ t; 194: box.error.MULTIKEY_INDEX_MISMATCH 195: box.error.CREATE_CK_CONSTRAINT 196: box.error.CK_CONSTRAINT_FAILED + 197: 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 76112cf..bc0dfb8 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/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 a8d3947..6362421 100755 --- a/test/sql-tap/sql-errors.test.lua +++ b/test/sql-tap/sql-errors.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(47) +test:plan(63) test:execsql([[ CREATE TABLE t0 (i INT PRIMARY KEY, a INT); @@ -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" -- }) @@ -526,4 +526,164 @@ test:do_catchsql_test( -- }) +test:do_catchsql_test( + "sql-errors-1.48", + [[ + 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( + "sql-errors-1.49", + [[ + 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( + "sql-errors-1.50", + [[ + 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( + "sql-errors-1.51", + [[ + 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( + "sql-errors-1.52", + [[ + SELECT (1) IN (1,2,3); + ]], { + -- + 0, {true} + -- + }) + +test:do_catchsql_test( + "sql-errors-1.53", + [[ + SELECT (1,2,3) IN (SELECT (1), (2), (3)); + ]], { + -- + 0, {true} + -- + }) + +test:do_catchsql_test( + "sql-errors-1.54", + [[ + SELECT (1,2,3) IN (VALUES (1,2,3), (2,3,4)); + ]], { + -- + 0, {true} + -- + }) + +test:do_catchsql_test( + "sql-errors-1.55", + [[ + SELECT (1,2,3) IN (SELECT * from (VALUES (1,2,3), (2,3,4))); + ]], { + -- + 0, {true} + -- + }) + +test:do_catchsql_test( + "sql-errors-1.56", + [[ + 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( + "sql-errors-1.57", + [[ + 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( + "sql-errors-1.58", + [[ + 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( + "sql-errors-1.59", + [[ + 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( + "sql-errors-1.60", + [[ + SELECT (1,2,3) = (1,2,3); + ]], { + -- + 0, {true} + -- + }) + +test:do_catchsql_test( + "sql-errors-1.61", + [[ + 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( + "sql-errors-1.62", + [[ + 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( + "sql-errors-1.63", + [[ + 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/subselect.test.lua b/test/sql-tap/subselect.test.lua index ebfdf43..3cf87a1 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" -- })