[tarantool-patches] Re: [PATCH 2/2 v2] sql: allow <COLLATE> only for string-like args

["n.pettik" <korablev@tarantool.org>]

>>> @@ -4396,6 +4433,8 @@ sqlExprCodeAtInit(Parse * pParse,	/* Parsing context */
>>> int
>>> sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg)
>>> {
>>> +	if (pExpr->op == TK_COLLATE && check_collate_arg(pParse, pExpr) != 0)
>>> +		return 0;
>> 
>> Why this check is required (in addition to the same check in ExprCodeTarget)?
> It is needed e.g. for the following queries: “SELECT NOT TRUE COLLATE “unicode””.
> “NOT” will be on the root. The sequence of calls: sqlExprCodeTagret -> (case TK_NOT)
> -> sqlExprCodeTemp.
> 			NOT
> 		       /   \
> 		   unicode 0x0
> 		  /      \
> 		TRUE	 0x0
>>> 	int r2;
>>> 	pExpr = sqlExprSkipCollate(pExpr);

In fact, problem is here: this function skips collate token.
So, diff is following:

@@ -4427,10 +4426,7 @@ sqlExprCodeAtInit(Parse * pParse,        /* Parsing context */
 int
 sqlExprCodeTemp(Parse * pParse, Expr * pExpr, int *pReg)
 {
-       if (pExpr->op == TK_COLLATE && check_collate_arg(pParse, pExpr) != 0)
-               return 0;
        int r2;
-       pExpr = sqlExprSkipCollate(pExpr);
        if (ConstFactorOk(pParse)
            && pExpr->op != TK_REGISTER && sqlExprIsConstantNotJoin(pExpr)
            ) {

What is more, you don’t need to set type of expression
in sqlExprCodeTarget():

@@ -4201,7 +4201,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
                }
        case TK_SPAN:
        case TK_COLLATE:{
-                       pExpr->pLeft->type = sql_expr_type(pExpr->pLeft);
                        if (check_collate_arg(pParse, pExpr) != 0)
                                break;
                        return sqlExprCodeTarget(pParse, pExpr->pLeft,

>>> diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
>>> index 1ca6a6446..1fdee16b7 100755
>>> --- a/test/sql-tap/in3.test.lua
>>> +++ b/test/sql-tap/in3.test.lua
>>> @@ -186,33 +186,6 @@ test:do_test(
>>>       -- </in3-1.13>
>>>   })
>>> 
>>> -
>>> -
>>> --- The first of these queries has to use the temp-table, because the 
>>> --- collation sequence used for the index on "t1.a" does not match the
>>> --- collation sequence used by the "IN" comparison. The second does not
>>> --- require a temp-table, because the collation sequences match.
>>> ---
>>> -test:do_test(
>>> -    "in3-1.14",
>>> -    function()
>>> -        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"unicode_ci\" IN (SELECT a FROM t1) ")
>>> -    end, {
>>> -        -- <in3-1.14>
>>> -        1, 1, 3, 5
>>> -        -- </in3-1.14>
>>> -    })
>>> -
>>> -test:do_test(
>>> -    "in3-1.15",
>>> -    function()
>>> -        return exec_neph(" SELECT a FROM t1 WHERE a COLLATE \"binary\" IN (SELECT a FROM t1) ")
>>> -    end, {
>>> -        -- <in3-1.15>
>>> -        1, 1, 3, 5
>>> -        -- </in3-1.15>
>>> -    })
>> 
>> I’d better fix these tests rather than delete.
> 
> Previous test in this file will be same if I remove COLLATE clauses.

You don’t have to remove collate clause, you should create new
table with string field type and run this test using that table.
Comment above the test clearly indicates its purpose.