[tarantool-patches] Re: [PATCH v1 1/1] sql: rework SQL errors of type "expected column count"

[Mergen Imeev <imeevma@tarantool.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;
     ]], {
         -- <in-9.4>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-9.4>
     })
 
@@ -677,7 +677,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <in-12.2>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-12.2>
     })
 
@@ -689,7 +689,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <in-12.3>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-12.3>
     })
 
@@ -701,7 +701,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <in-12.4>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-12.4>
     })
 
@@ -713,7 +713,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <in-12.5>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-12.5>
     })
 
@@ -823,7 +823,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <in-12.14>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </in-12.14>
     })
 
@@ -1058,7 +1058,7 @@ test:do_catchsql_test(
         SELECT 0 WHERE (SELECT 0,0) OR (0 IN (1,2));
     ]], {
         -- <in-13.15>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 2, but right side - 1"
         -- </in-13.15>
     })
 
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);
     ]], {
         -- <select7-5.1>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </select7-5.1>
     })
 
@@ -140,7 +140,7 @@ test:do_catchsql_test(
         SELECT 5 IN (SELECT * FROM t2);
     ]], {
         -- <select7-5.2>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </select7-5.2>
     })
 
@@ -150,7 +150,7 @@ test:do_catchsql_test(
         SELECT 5 IN (SELECT a,b FROM t2 UNION SELECT b,a FROM t2);
     ]], {
         -- <select7-5.3>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </select7-5.3>
     })
 
@@ -160,7 +160,7 @@ test:do_catchsql_test(
         SELECT 5 IN (SELECT * FROM t2 UNION SELECT * FROM t2);
     ]], {
         -- <select7-5.4>
-        1, "sub-select returns 2 columns - expected 1"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </select7-5.4>
     })
 
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);
 	]], {
 		-- <sql-errors-1.41>
-		1,"row value misused"
+		1,"Unequal number of entries in row expression: left side has 3, but right side - 4"
 		-- </sql-errors-1.41>
 	})
 
@@ -526,4 +526,164 @@ test:do_catchsql_test(
 		-- </sql-errors-1.47>
 	})
 
+test:do_catchsql_test(
+	"sql-errors-1.48",
+	[[
+		SELECT (1,2) IN (1,2,3);
+	]], {
+		-- <sql-errors-1.48>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 1"
+		-- </sql-errors-1.48>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.49",
+	[[
+		SELECT (1,2) IN (SELECT (1), (2), (3));
+	]], {
+		-- <sql-errors-1.49>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.49>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.50",
+	[[
+		SELECT (1,2) IN (VALUES (1,2,3), (2,3,4));
+	]], {
+		-- <sql-errors-1.50>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.50>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.51",
+	[[
+		SELECT (1,2) IN (SELECT * from (VALUES (1,2,3), (2,3,4)));
+	]], {
+		-- <sql-errors-1.51>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.51>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.52",
+	[[
+		SELECT (1) IN (1,2,3);
+	]], {
+		-- <sql-errors-1.52>
+		0, {true}
+		-- </sql-errors-1.52>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.53",
+	[[
+		SELECT (1,2,3) IN (SELECT (1), (2), (3));
+	]], {
+		-- <sql-errors-1.53>
+		0, {true}
+		-- </sql-errors-1.53>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.54",
+	[[
+		SELECT (1,2,3) IN (VALUES (1,2,3), (2,3,4));
+	]], {
+		-- <sql-errors-1.54>
+		0, {true}
+		-- </sql-errors-1.54>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.55",
+	[[
+		SELECT (1,2,3) IN (SELECT * from (VALUES (1,2,3), (2,3,4)));
+	]], {
+		-- <sql-errors-1.55>
+		0, {true}
+		-- </sql-errors-1.55>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.56",
+	[[
+		SELECT (SELECT * FROM (VALUES (1,2))) IN (1,2,3);
+	]], {
+		-- <sql-errors-1.56>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 1"
+		-- </sql-errors-1.56>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.57",
+	[[
+		SELECT (SELECT * FROM (VALUES (1,2))) IN (SELECT (1), (2), (3));
+	]], {
+		-- <sql-errors-1.57>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.57>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.58",
+	[[
+		SELECT (SELECT * FROM (VALUES (1,2))) IN (VALUES (1,2,3), (2,3,4));
+	]], {
+		-- <sql-errors-1.58>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.58>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.59",
+	[[
+		SELECT (SELECT * FROM (VALUES (1,2))) IN (SELECT * from (VALUES (1,2,3), (2,3,4)));
+	]], {
+		-- <sql-errors-1.59>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.59>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.60",
+	[[
+		SELECT (1,2,3) = (1,2,3);
+	]], {
+		-- <sql-errors-1.60>
+		0, {true}
+		-- </sql-errors-1.60>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.61",
+	[[
+		SELECT (1,2) = (1,2,3);
+	]], {
+		-- <sql-errors-1.61>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.61>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.62",
+	[[
+		SELECT (SELECT (1), (2)) = (1,2,3);
+	]], {
+		-- <sql-errors-1.62>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.62>
+	})
+
+test:do_catchsql_test(
+	"sql-errors-1.63",
+	[[
+		SELECT (VALUES (1,2)) = (1,2,3);
+	]], {
+		-- <sql-errors-1.63>
+		1, "Unequal number of entries in row expression: left side has 2, but right side - 3"
+		-- </sql-errors-1.63>
+	})
+
 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)
     ]], {
         -- <subselect-1.2>
-        1, "row value misused"
+        1, "Unequal number of entries in row expression: left side has 1, but right side - 2"
         -- </subselect-1.2>
     })