From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AD15046970E for ; Mon, 10 Feb 2020 13:54:41 +0300 (MSK) Date: Mon, 10 Feb 2020 13:54:39 +0300 From: Mergen Imeev Message-ID: <20200210105438.GA3715@tarantool.org> References: <20200207113440.79470-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200207113440.79470-1-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: fix bug type mismatch error List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org 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 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( > -- > }) > > -test:do_catchsql_test( > +test:do_execsql_test( > "tkt-80e031a00f.27", > [[ > SELECT 'hello' IN t1 > ]], { > -- > - 1, 'Type mismatch: can not convert hello to integer' > + false > -- > }) > > -test:do_catchsql_test( > +test:do_execsql_test( > "tkt-80e031a00f.28", > [[ > SELECT 'hello' NOT IN t1 > ]], { > -- > - 1, 'Type mismatch: can not convert hello to integer' > + true > -- > }) > > @@ -380,23 +380,23 @@ test:do_execsql_test( > -- > }) > > -test:do_catchsql_test( > +test:do_execsql_test( > "tkt-80e031a00f.31", > [[ > SELECT x'303132' IN t1 > ]], { > -- > - 1, 'Type mismatch: can not convert varbinary to integer' > + false > -- > }) > > -test:do_catchsql_test( > +test:do_execsql_test( > "tkt-80e031a00f.32", > [[ > SELECT x'303132' NOT IN t1 > ]], { > -- > - 1, 'Type mismatch: can not convert varbinary to integer' > + true > -- > }) > > 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) >