[tarantool-patches] Re: [PATCH v1 1/1] sql: rework SQL errors of type "expected column count"
Mergen Imeev
imeevma at tarantool.org
Sat May 25 14:12:34 MSK 2019
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 at 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;
]], {
-- <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 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);
]], {
-- <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 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)
]], {
-- <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>
})
More information about the Tarantool-patches
mailing list