[tarantool-patches] Re: [PATCH] sql: make IN operator stop ignoring type and collation

[Vladislav Shpilevoy]

LGTM

On 16/01/2019 16:34, Nikita Pettik wrote:
> SQLite discards type and collation of IN operator when it comes with
> only one operand. This leads to different results of straight comparison
> using '=' operator and IN:
> 
> SELECT x FROM t1 WHERE x IN (1.0);
> -- Result is empty set
> SELECT x FROM t1 WHERE x = 1.0;
> - - ['1']
> 
> Lets remove this strange ignorance and always take into consideration
> types and collations of operands.
> 
> Closes #3934
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix
> Issue: https://github.com/tarantool/tarantool/issues/3934
> 
>   src/box/sql/expr.c                   |  4 ----
>   src/box/sql/parse.y                  | 11 -----------
>   src/box/sql/sqliteInt.h              |  1 -
>   test/sql-tap/in4.test.lua            |  8 ++++----
>   test/sql-tap/tkt-9a8b09f8e6.test.lua |  7 ++++---
>   5 files changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b67b22c23..d83be5101 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -73,8 +73,6 @@ char
>   sqlite3ExprAffinity(Expr * pExpr)
>   {
>   	pExpr = sqlite3ExprSkipCollate(pExpr);
> -	if (pExpr->flags & EP_Generic)
> -		return 0;
>   	uint8_t op = pExpr->op;
>   	struct ExprList *el;
>   	if (op == TK_REGISTER)
> @@ -197,8 +195,6 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
>   	*coll_id = COLL_NONE;
>   	while (p != NULL) {
>   		int op = p->op;
> -		if (p->flags & EP_Generic)
> -			break;
>   		if (op == TK_CAST || op == TK_UPLUS) {
>   			p = p->pLeft;
>   			continue;
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 50bb2ba01..0bcf41594 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1101,21 +1101,10 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
>       **
>       **      expr1 == ?1
>       **      expr1 <> ?2
> -    **
> -    ** But, the RHS of the == or <> is marked with the EP_Generic flag
> -    ** so that it may not contribute to the computation of comparison
> -    ** affinity or the collating sequence to use for comparison.  Otherwise,
> -    ** the semantics would be subtly different from IN or NOT IN.
>       */
>       Expr *pRHS = Y->a[0].pExpr;
>       Y->a[0].pExpr = 0;
>       sql_expr_list_delete(pParse->db, Y);
> -    /* pRHS cannot be NULL because a malloc error would have been detected
> -    ** before now and control would have never reached this point */
> -    if( ALWAYS(pRHS) ){
> -      pRHS->flags &= ~EP_Collate;
> -      pRHS->flags |= EP_Generic;
> -    }
>       A.pExpr = sqlite3PExpr(pParse, N ? TK_NE : TK_EQ, A.pExpr, pRHS);
>     }else{
>       A.pExpr = sqlite3PExpr(pParse, TK_IN, A.pExpr, 0);
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 7e16edc9a..ee24e0337 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2158,7 +2158,6 @@ struct Expr {
>   #define EP_DblQuoted 0x000040	/* token.z was originally in "..." */
>   #define EP_InfixFunc 0x000080	/* True for an infix function: LIKE, etc */
>   #define EP_Collate   0x000100	/* Tree contains a TK_COLLATE operator */
> -#define EP_Generic   0x000200	/* Ignore COLLATE or affinity on this tree */
>   #define EP_IntValue  0x000400	/* Integer value contained in u.iValue */
>   #define EP_xIsSelect 0x000800	/* x.pSelect is valid (otherwise x.pList is) */
>   #define EP_Skip      0x001000	/* COLLATE, AS, or UNLIKELY */
> diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
> index b029535af..c488cde45 100755
> --- a/test/sql-tap/in4.test.lua
> +++ b/test/sql-tap/in4.test.lua
> @@ -518,8 +518,8 @@ test:do_execsql_test(
>       })
>   
>   -- MUST_WORK_TEST
> --- Make sure that when "x IN (?)" is converted into "x==?" that collating
> --- sequence and affinity computations do not get messed up.
> +-- Make sure that when "x IN (?)" is converted into "x==?":
> +-- type and collation sequence should be taken into consideration.
>   --
>   test:do_execsql_test(
>       "in4-4.1",
> @@ -571,7 +571,7 @@ test:do_execsql_test(
>           SELECT c FROM t4a WHERE a IN (b) ORDER BY c;
>       ]], {
>           -- <in4-4.5>
> -        3
> +        1, 3
>           -- </in4-4.5>
>       })
>   
> @@ -581,7 +581,7 @@ test:do_execsql_test(
>           SELECT c FROM t4a WHERE (a||'') IN (b) ORDER BY c;
>       ]], {
>           -- <in4-4.6>
> -        3
> +        1, 3
>           -- </in4-4.6>
>       })
>   
> diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> index 4d08cd657..996809afe 100755
> --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
> +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> @@ -99,7 +99,7 @@ test:do_execsql_test(
>           SELECT x FROM t1 WHERE x IN (1.0);
>       ]], {
>           -- <2.2>
> -
> +        "1"
>           -- </2.2>
>       })
>   
> @@ -239,7 +239,7 @@ test:do_execsql_test(
>           SELECT x FROM t2 WHERE '1.0' IN (x);
>       ]], {
>           -- <3.8>
> -
> +        1
>           -- </3.8>
>       })
>   
> @@ -309,7 +309,7 @@ test:do_execsql_test(
>           SELECT x FROM t3 WHERE '1' IN (x);
>       ]], {
>           -- <4.7>
> -
> +        1
>           -- </4.7>
>       })
>   
> @@ -519,6 +519,7 @@ test:do_execsql_test(
>           SELECT x, y FROM t5 WHERE '1.0' IN (x);
>       ]], {
>           -- <6.8>
> +        1, "one", 1, "two", 1, "three", 1, "four"
>           -- </6.8>
>       })
>   
>