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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi!

tarantool> box.execute('SELECT TRUE AND TRUE COLLATE "binary";')
---
- metadata:
  - name: TRUE AND TRUE COLLATE "binary"
    type: boolean
  rows:
  - [true]
...

Your previous solution was correct, you should check collation
in CodeTemp. But somewhy you decided to remove it and keep only
in 'TK_NOT' and 'TK_COLLATE'. Obviously, you can't add the check
to each 'case' in that 'switch', and you need to check the collation
for every token.

Besides, Nikita, please, help us with the next comment.

On 04/06/2019 17:27, Roman Khabibov wrote:
> Hello, thanks for the review.
> 
>> On Jun 2, 2019, at 8:09 PM, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Thanks for the fixes! See 8 comments below.
>>
>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>> index ba7eea59d..29e3386fa 100644
>>>>> --- a/src/box/sql/expr.c
>>>>> +++ b/src/box/sql/expr.c
>>>>> @@ -4215,6 +4215,22 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>>>> 		}
>>>>> 	case TK_SPAN:
>>>>> 	case TK_COLLATE:{
>>>>> +			enum field_type type;
>>>>> +			struct Expr *left = pExpr->pLeft;
>>>>> +			if (left->op == TK_COLUMN) {
>>>>> +				int col_num = left->iColumn;
>>>>> +				type = left->space_def->fields[col_num].type;
>>>>> +			} else
>>>>> +				type = left->type;
>>>>> +			if (left->op != TK_CONCAT &&
>>>>
>>>> Why do you check for TK_CONCAT? Its type should be FIELD_TYPE_STRING.
>>> Concatenations are rejected without this check.
>>
>> 1. It is not a good answer, when you can't explain, why you need
>> a line of code, but a program does not work without it. Please, investigate
>> and explain, why do you check for both 'type == STRING' and 'op == CONCAT'.
>> If an op is CONCAT, then type is already STRING, so the additional check
>> for 'op' should not be necessary.
> @@ -1060,6 +1094,8 @@ sqlPExpr(Parse * pParse,	/* Parsing context */
>  		if (p) {
>  			memset(p, 0, sizeof(Expr));
>  			p->op = op & TKFLG_MASK;
> +			if (op == TK_CONCAT)
> +				p->type = FIELD_TYPE_STRING;
>  			p->iAgg = -1;
>  		}
>  		sqlExprAttachSubtrees(pParse->db, p, pLeft, pRight);
> 

I am 100% sure, that it is not a place for types setting. This
function, and its comment says, only allocates. For complex types
we have sql_expr_type(), we can use Expr.type for basic types
located in tree lists only.

Nikita knows more about typing, he can help better.