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 C06812DDD2 for ; Sat, 25 May 2019 07:12:38 -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 T0HrKclH-I2o for ; Sat, 25 May 2019 07:12:38 -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 705202DD8E for ; Sat, 25 May 2019 07:12:37 -0400 (EDT) Date: Sat, 25 May 2019 14:12:34 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: rework SQL errors of type "expected column count" Message-ID: <20190525111234.GB9859@tarantool.org> References: <461ca6b5a31331d36ef7d23872613f0b1c0de9d0.1558176783.git.imeevma@gmail.com> <70E77545-041F-418E-828C-335A449EF4A6@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <70E77545-041F-418E-828C-335A449EF4A6@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 Hi! Thank you for review! My answers and new patch below. On Sun, May 19, 2019 at 05:38:16PM +0300, n.pettik wrote: > > > > On 18 May 2019, at 13:53, imeevma@tarantool.org wrote: > > > > This patch reworks SQL errors of type "expected column count”. > > Please, provide decent description of the patch. > Fixed. > > diff --git a/src/box/errcode.h b/src/box/errcode.h > > index 9c15f33..789556a 100644 > > --- a/src/box/errcode.h > > +++ b/src/box/errcode.h > > @@ -247,6 +247,7 @@ struct errcode_record { > > /*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_MULTIKEY_INDEX_MISMATCH, "Field %s is used as multikey in one index and as single key in another") \ > > + /*195 */_(ER_SQL_COLUMN_COUNT, "Left value column count %u does not match the expected column count (%u)") \ > > -> ER_SQL_ROW_EXPRESSION > > I would re-phrase message to smth like this: > > “Misuse of row expression: left side of row value has %u entries, but right side - %u” > “Unequal number of entries in row expression: left side has %u, but right side - %u” > Thanks. I used the second option. > > /* > > * !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. 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. Closes #4204 diff --git a/src/box/errcode.h b/src/box/errcode.h index 9c15f33..c0042b5 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -247,6 +247,7 @@ struct errcode_record { /*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_MULTIKEY_INDEX_MISMATCH, "Field %s is used as multikey in one index and as single key in another") \ + /*195 */_(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 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. */ 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; } @@ -4153,10 +4147,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 504096e..3d45841 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -781,15 +781,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 d3472a9..69b888e 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -3616,12 +3616,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 4fcd13a..766bf9c 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -523,6 +523,7 @@ t; 192: box.error.INDEX_DEF_UNSUPPORTED 193: box.error.CK_DEF_UNSUPPORTED 194: box.error.MULTIKEY_INDEX_MISMATCH + 195: 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 9357c40..da10ecb 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 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" -- })