From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 02AA52DF11 for ; Tue, 4 Jun 2019 15:49:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bTVpG9P3a8VO for ; Tue, 4 Jun 2019 15:49:12 -0400 (EDT) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B47652FE41 for ; Tue, 4 Jun 2019 15:49:12 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: allow only for string-like args References: <015351a65570062bdd3577b7222f047f7de7e89b.1557312975.git.roman.habibov@tarantool.org> <1abf945a-736f-735a-98c5-a85db184ddb5@tarantool.org> <4389A940-D474-4977-A47A-FFBF809C3CC3@tarantool.org> <6bb13018-d6f4-75b0-97bf-19a4d2613f38@tarantool.org> From: Vladislav Shpilevoy Message-ID: <545e9807-0558-8ce2-cacf-b96eac63e0d1@tarantool.org> Date: Tue, 4 Jun 2019 21:49:07 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Roman Khabibov , tarantool-patches@freelists.org, "n.pettik" 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 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.