[Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error
Mergen Imeev
imeevma at tarantool.org
Mon Feb 10 13:54:39 MSK 2020
Hello! Thanks for the patch. In general, I think that part of the
problem will be solved along with the solution to problem #4230
("sql: implicit type cast during comparison"). But since #4230 is
still under discussion, it might be better to fix the bug with
your patch. Still, I think that most of this patch will be
rewritten in #4230.
See 6 comments below.
On Fri, Feb 07, 2020 at 02:34:40PM +0300, Roman Khabibov wrote:
> Ignore type mismatch error in the certain VDBE operations, if they
> are used to process <IN> predicate.
>
1. I not think that the patch name is appropriate.
> Closes #4692
> ---
> Branch: https://github.com/tarantool/tarantool/pull/new/romanhabibov/gh-4692-in-bug
> Issue: https://github.com/tarantool/tarantool/issues/4692
> src/box/sql/expr.c | 27 ++++++++----
> src/box/sql/parse.y | 2 +
> src/box/sql/sqlInt.h | 7 ++++
> src/box/sql/vdbe.c | 9 ++--
> test/sql-tap/tkt-80e031a00f.test.lua | 16 +++----
> test/sql/boolean.result | 42 +++++++++++++------
> test/sql/gh-4692-in-bug.result | 62 ++++++++++++++++++++++++++++
> test/sql/gh-4692-in-bug.test.lua | 17 ++++++++
> 8 files changed, 151 insertions(+), 31 deletions(-)
> create mode 100644 test/sql/gh-4692-in-bug.result
> create mode 100755 test/sql/gh-4692-in-bug.test.lua
>
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index bc2182446..ee72b90d9 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -425,12 +425,12 @@ expr_cmp_mutual_type(struct Expr *pExpr)
> * Return the P5 value that should be used for a binary comparison
> * opcode (OP_Eq, OP_Ge etc.) used to compare pExpr1 and pExpr2.
> */
> -static u8
> +static u16
> binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull)
> {
> enum field_type lhs = sql_expr_type(pExpr2);
> enum field_type rhs = sql_expr_type(pExpr1);
> - u8 type_mask = sql_type_result(rhs, lhs) | (u8) jumpIfNull;
> + u16 type_mask = sql_type_result(rhs, lhs) | (u16) jumpIfNull;
> return type_mask;
> }
>
2. I don't think we should use u8, u16 and similar types in
our code. Why not just use int?
> @@ -515,7 +515,7 @@ codeCompare(Parse * pParse, /* The parsing (and code generating) context */
> int p5 = binaryCompareP5(pLeft, pRight, jumpIfNull);
> int addr = sqlVdbeAddOp4(pParse->pVdbe, opcode, in2, dest, in1,
> (void *)coll, P4_COLLSEQ);
> - sqlVdbeChangeP5(pParse->pVdbe, (u8) p5);
> + sqlVdbeChangeP5(pParse->pVdbe, p5);
> return addr;
> }
>
> @@ -736,6 +736,8 @@ codeVectorCompare(Parse * pParse, /* Code generator context */
>
> u8 opx;
> u8 p5 = SQL_STOREP2;
> + if ((pExpr->flags & EP_IgnoreMismatch) != 0)
> + p5 |= SQL_IGNORE_MISMATCH;
3. What is this for? After removing this code and running
the tests, I did not get any errors.
> if (op == TK_LE)
> opx = TK_LT;
> else if (op == TK_GE)
> @@ -3140,16 +3142,16 @@ sqlExprCodeIN(Parse * pParse, /* Parsing and code generating context */
> (void *)coll, P4_COLLSEQ);
> VdbeCoverageIf(v, ii < pList->nExpr - 1);
> VdbeCoverageIf(v, ii == pList->nExpr - 1);
> - sqlVdbeChangeP5(v, zAff[0]);
> + sqlVdbeChangeP5(v, zAff[0] |
> + SQL_IGNORE_MISMATCH);
> } else {
> assert(destIfNull == destIfFalse);
> sqlVdbeAddOp4(v, OP_Ne, rLhs, destIfFalse,
> r2, (void *)coll,
> P4_COLLSEQ);
> VdbeCoverage(v);
> - sqlVdbeChangeP5(v,
> - zAff[0] |
> - SQL_JUMPIFNULL);
> + sqlVdbeChangeP5(v, zAff[0] | SQL_JUMPIFNULL |
> + SQL_IGNORE_MISMATCH);
> }
> sqlReleaseTempReg(pParse, regToFree);
> }
> @@ -3187,6 +3189,7 @@ sqlExprCodeIN(Parse * pParse, /* Parsing and code generating context */
> zAff[nVector] = field_type_MAX;
> sqlVdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char*)zAff,
> P4_DYNAMIC);
> + sqlVdbeChangeP5(v, SQL_IGNORE_MISMATCH);
4. The only use of OP_ApplyType here is to implicitly cast
from a number to STRING and from STRING to a number. I'm
not sure, that we should do this.
I will ask if we should do this for the IN operator in the
"Implicit cast for COMPARISON" thread.
> /*
> * zAff will be freed at the end of VDBE execution, since
> * it was passed with P4_DYNAMIC flag.
> @@ -3252,6 +3255,7 @@ sqlExprCodeIN(Parse * pParse, /* Parsing and code generating context */
> sqlVdbeAddOp3(v, OP_Column, pExpr->iTable, aiMap[i], r3);
> sqlVdbeAddOp4(v, OP_Ne, rLhs + i, destNotNull, r3,
> (void *)pColl, P4_COLLSEQ);
> + sqlVdbeChangeP5(v, SQL_IGNORE_MISMATCH);
> VdbeCoverage(v);
> sqlReleaseTempReg(pParse, r3);
> }
> @@ -3824,8 +3828,11 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> ®Free1);
> r2 = sqlExprCodeTemp(pParse, pExpr->pRight,
> ®Free2);
> + int flags = SQL_STOREP2;
> + if ((pExpr->flags & EP_IgnoreMismatch) != 0)
> + flags |= SQL_IGNORE_MISMATCH;
> codeCompare(pParse, pLeft, pExpr->pRight, op,
> - r1, r2, inReg, SQL_STOREP2);
> + r1, r2, inReg, flags);
> assert(TK_LT == OP_Lt);
> testcase(op == OP_Lt);
> VdbeCoverageIf(v, op == OP_Lt);
> @@ -4755,6 +4762,8 @@ sqlExprIfTrue(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
> if (sqlExprIsVector(pExpr->pLeft))
> goto default_expr;
> testcase(jumpIfNull == 0);
> + if ((pExpr->flags & EP_IgnoreMismatch) != 0)
> + jumpIfNull |= SQL_IGNORE_MISMATCH;
> r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
> ®Free1);
> r2 = sqlExprCodeTemp(pParse, pExpr->pRight,
> @@ -4948,6 +4957,8 @@ sqlExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
> if (sqlExprIsVector(pExpr->pLeft))
> goto default_expr;
> testcase(jumpIfNull == 0);
> + if ((pExpr->flags & EP_IgnoreMismatch) != 0)
> + jumpIfNull |= SQL_IGNORE_MISMATCH;
> r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
> ®Free1);
> r2 = sqlExprCodeTemp(pParse, pExpr->pRight,
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index cfe1c0012..679758d4c 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1383,6 +1383,8 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
> Y->a[0].pExpr = 0;
> sql_expr_list_delete(pParse->db, Y);
> A.pExpr = sqlPExpr(pParse, N ? TK_NE : TK_EQ, A.pExpr, pRHS);
> + if (A.pExpr != NULL)
> + A.pExpr->flags |= EP_IgnoreMismatch;
> }else{
> A.pExpr = sqlPExpr(pParse, TK_IN, A.pExpr, 0);
> if( A.pExpr ){
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d1fcf4761..1a9d6961d 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -1318,6 +1318,11 @@ enum trim_side_mask {
> #define SQL_KEEPNULL 0x40 /* Used by vector == or <> */
> #define SQL_NULLEQ 0x80 /* NULL=NULL */
> #define SQL_NOTNULL 0x90 /* Assert that operands are never NULL */
> +/*
> + * Don't throw ER_SQL_TYPE_MISMATCH when compared types are
> + * different.
> + */
> +#define SQL_IGNORE_MISMATCH 0x100
5. I think it is wrong to use this flag in comparison
operations and in OP_ApplyType. This makes the flag
something between the flags for one OP group and the global
flags.
In case we decide to remove OP_ApplyType from IN (see 4.),
the flag will remain only in comparison operations. Then it
will be better, in my opinion.
>
> /**
> * Return logarithm of tuple count in space.
> @@ -1632,6 +1637,8 @@ struct Expr {
> #define EP_Leaf 0x800000 /* Expr.pLeft, .pRight, .u.pSelect all NULL */
> /** Expression is system-defined. */
> #define EP_System 0x1000000
> +/** Ignore type mismatch error. */
> +#define EP_IgnoreMismatch 0x2000000
6. I think we can try to remove this flag using the SCALAR
rules for IN for comparison. However, this may be outside
of your patch.
>
> /*
> * Combinations of two or more EP_* flags
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index eab74db4a..b201440d6 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2191,7 +2191,8 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> */
> int type_arg1 = flags1 & (MEM_Bool | MEM_Blob);
> int type_arg3 = flags3 & (MEM_Bool | MEM_Blob);
> - if (type_arg1 != type_arg3) {
> + if (type_arg1 != type_arg3 &&
> + (pOp->p5 & SQL_IGNORE_MISMATCH) == 0) {
> char *inconsistent_type = type_arg1 != 0 ?
> mem_type_to_str(pIn3) :
> mem_type_to_str(pIn1);
> @@ -2213,7 +2214,8 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> flags3 = pIn3->flags;
> }
> if ((flags3 & MEM_Str) == MEM_Str) {
> - if (mem_apply_numeric_type(pIn3) != 0) {
> + if (mem_apply_numeric_type(pIn3) != 0 &&
> + (pOp->p5 & SQL_IGNORE_MISMATCH) == 0) {
> diag_set(ClientError,
> ER_SQL_TYPE_MISMATCH,
> sql_value_to_diag_str(pIn3),
> @@ -2779,7 +2781,8 @@ case OP_ApplyType: {
> while((type = *(types++)) != field_type_MAX) {
> assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> assert(memIsValid(pIn1));
> - if (mem_apply_type(pIn1, type) != 0) {
> + if (mem_apply_type(pIn1, type) != 0 &&
> + (pOp->p5 & SQL_IGNORE_MISMATCH) == 0) {
> diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> sql_value_to_diag_str(pIn1),
> field_type_strs[type]);
> diff --git a/test/sql-tap/tkt-80e031a00f.test.lua b/test/sql-tap/tkt-80e031a00f.test.lua
> index a0e6539e0..8553a15a8 100755
> --- a/test/sql-tap/tkt-80e031a00f.test.lua
> +++ b/test/sql-tap/tkt-80e031a00f.test.lua
> @@ -340,23 +340,23 @@ test:do_execsql_test(
> -- </tkt-80e031a00f.26>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> "tkt-80e031a00f.27",
> [[
> SELECT 'hello' IN t1
> ]], {
> -- <tkt-80e031a00f.27>
> - 1, 'Type mismatch: can not convert hello to integer'
> + false
> -- </tkt-80e031a00f.27>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> "tkt-80e031a00f.28",
> [[
> SELECT 'hello' NOT IN t1
> ]], {
> -- <tkt-80e031a00f.28>
> - 1, 'Type mismatch: can not convert hello to integer'
> + true
> -- </tkt-80e031a00f.28>
> })
>
> @@ -380,23 +380,23 @@ test:do_execsql_test(
> -- </tkt-80e031a00f.30>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> "tkt-80e031a00f.31",
> [[
> SELECT x'303132' IN t1
> ]], {
> -- <tkt-80e031a00f.31>
> - 1, 'Type mismatch: can not convert varbinary to integer'
> + false
> -- </tkt-80e031a00f.31>
> })
>
> -test:do_catchsql_test(
> +test:do_execsql_test(
> "tkt-80e031a00f.32",
> [[
> SELECT x'303132' NOT IN t1
> ]], {
> -- <tkt-80e031a00f.32>
> - 1, 'Type mismatch: can not convert varbinary to integer'
> + true
> -- </tkt-80e031a00f.32>
> })
>
> diff --git a/test/sql/boolean.result b/test/sql/boolean.result
> index 7769d0cb3..8b3869879 100644
> --- a/test/sql/boolean.result
> +++ b/test/sql/boolean.result
> @@ -3871,13 +3871,19 @@ SELECT false IN (0, 1, 2, 3);
> | ...
> SELECT true IN (SELECT b FROM t7);
> | ---
> - | - null
> - | - 'Type mismatch: can not convert TRUE to integer'
> + | - metadata:
> + | - name: true IN (SELECT b FROM t7)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
> SELECT false IN (SELECT b FROM t7);
> | ---
> - | - null
> - | - 'Type mismatch: can not convert FALSE to integer'
> + | - metadata:
> + | - name: false IN (SELECT b FROM t7)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
> SELECT a1, a1 IN (0, 1, 2, 3) FROM t6
> | ---
> @@ -5032,23 +5038,35 @@ SELECT a2 IN (0.1, 1.2, 2.3, 3.4) FROM t6 LIMIT 1;
> | ...
> SELECT true IN (SELECT c FROM t8);
> | ---
> - | - null
> - | - 'Type mismatch: can not convert TRUE to number'
> + | - metadata:
> + | - name: true IN (SELECT c FROM t8)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
> SELECT false IN (SELECT c FROM t8);
> | ---
> - | - null
> - | - 'Type mismatch: can not convert FALSE to number'
> + | - metadata:
> + | - name: false IN (SELECT c FROM t8)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
> SELECT a1 IN (SELECT c FROM t8) FROM t6 LIMIT 1;
> | ---
> - | - null
> - | - 'Type mismatch: can not convert FALSE to number'
> + | - metadata:
> + | - name: a1 IN (SELECT c FROM t8)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
> SELECT a2 IN (SELECT c FROM t8) FROM t6 LIMIT 1;
> | ---
> - | - null
> - | - 'Type mismatch: can not convert TRUE to number'
> + | - metadata:
> + | - name: a2 IN (SELECT c FROM t8)
> + | type: boolean
> + | rows:
> + | - [false]
> | ...
>
> SELECT true BETWEEN 0.1 and 9.9;
> diff --git a/test/sql/gh-4692-in-bug.result b/test/sql/gh-4692-in-bug.result
> new file mode 100644
> index 000000000..b0a7db6c5
> --- /dev/null
> +++ b/test/sql/gh-4692-in-bug.result
> @@ -0,0 +1,62 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +
> +--
> +-- gh-4692: Make sure that there is no type mismatch error.
> +--
> +box.execute("CREATE TABLE t1(a INT PRIMARY KEY);")
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute("SELECT a FROM t1 WHERE a IN (a+8,true);")
> + | ---
> + | - metadata:
> + | - name: A
> + | type: integer
> + | rows: []
> + | ...
> +box.execute("SELECT true IN (1);")
> + | ---
> + | - metadata:
> + | - name: true IN (1)
> + | type: boolean
> + | rows:
> + | - [false]
> + | ...
> +box.execute("SELECT true IN (VALUES(1));")
> + | ---
> + | - metadata:
> + | - name: true IN (VALUES(1))
> + | type: boolean
> + | rows:
> + | - [false]
> + | ...
> +box.execute("SELECT true IN (1, 2);")
> + | ---
> + | - metadata:
> + | - name: true IN (1, 2)
> + | type: boolean
> + | rows:
> + | - [false]
> + | ...
> +box.execute("SELECT true IN (VALUES(1), (2), ('3'));")
> + | ---
> + | - metadata:
> + | - name: true IN (VALUES(1), (2), ('3'))
> + | type: boolean
> + | rows:
> + | - [false]
> + | ...
> +
> +--
> +-- Cleanup.
> +--
> +box.execute('DROP TABLE t1;')
> + | ---
> + | - row_count: 1
> + | ...
> diff --git a/test/sql/gh-4692-in-bug.test.lua b/test/sql/gh-4692-in-bug.test.lua
> new file mode 100755
> index 000000000..fa52615a6
> --- /dev/null
> +++ b/test/sql/gh-4692-in-bug.test.lua
> @@ -0,0 +1,17 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +
> +--
> +-- gh-4692: Make sure that there is no type mismatch error.
> +--
> +box.execute("CREATE TABLE t1(a INT PRIMARY KEY);")
> +box.execute("SELECT a FROM t1 WHERE a IN (a+8,true);")
> +box.execute("SELECT true IN (1);")
> +box.execute("SELECT true IN (VALUES(1));")
> +box.execute("SELECT true IN (1, 2);")
> +box.execute("SELECT true IN (VALUES(1), (2), ('3'));")
> +
> +--
> +-- Cleanup.
> +--
> +box.execute('DROP TABLE t1;')
> --
> 2.21.0 (Apple Git-122)
>
More information about the Tarantool-patches
mailing list