[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)
>  							 &regFree1);
>  				r2 = sqlExprCodeTemp(pParse, pExpr->pRight,
>  							 &regFree2);
> +				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,
>  						 &regFree1);
>  			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,
>  						 &regFree1);
>  			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