* [Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error
@ 2020-02-07 11:34 Roman Khabibov
2020-02-10 10:54 ` Mergen Imeev
0 siblings, 1 reply; 3+ messages in thread
From: Roman Khabibov @ 2020-02-07 11:34 UTC (permalink / raw)
To: tarantool-patches
Ignore type mismatch error in the certain VDBE operations, if they
are used to process <IN> predicate.
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;
}
@@ -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;
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);
/*
* 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
/**
* 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
/*
* 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)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error
2020-02-07 11:34 [Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error Roman Khabibov
@ 2020-02-10 10:54 ` Mergen Imeev
2020-02-10 13:48 ` Nikita Pettik
0 siblings, 1 reply; 3+ messages in thread
From: Mergen Imeev @ 2020-02-10 10:54 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches
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)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error
2020-02-10 10:54 ` Mergen Imeev
@ 2020-02-10 13:48 ` Nikita Pettik
0 siblings, 0 replies; 3+ messages in thread
From: Nikita Pettik @ 2020-02-10 13:48 UTC (permalink / raw)
To: Mergen Imeev; +Cc: tarantool-patches
On 10 Feb 13:54, Mergen Imeev wrote:
> 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.
Guys, result of IN operation must be equal (in all senses) to the result
of one-by-one explicit comparisons:
SELECT true IN (1, 2, 3); <==> SELECT true == 1 OR true == 2 OR true == 3;
The latter raises an error, since there's no implicit conversion between
booleans and numerics. Hence, the goal of issue not only to make results
IN operations be equal, but rather make it return the same correct result
(i.e. as comparison does).
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-10 13:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07 11:34 [Tarantool-patches] [PATCH] sql: fix bug <IN> type mismatch error Roman Khabibov
2020-02-10 10:54 ` Mergen Imeev
2020-02-10 13:48 ` Nikita Pettik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox