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 E4EF12546A for ; Thu, 17 May 2018 07:56:47 -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 S6dXhsRMQ6xu for ; Thu, 17 May 2018 07:56:47 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 9D55125460 for ; Thu, 17 May 2018 07:56:47 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings References: <83d1862c-47af-118e-11cd-7c8ce786f468@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5c5e8260-38d6-1ae9-eb01-af25124b4dc5@tarantool.org> Date: Thu, 17 May 2018 14:56:44 +0300 MIME-Version: 1.0 In-Reply-To: <83d1862c-47af-118e-11cd-7c8ce786f468@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed 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: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. See 6 comments below. On 17/05/2018 13:39, Kirill Shcherbatov wrote: > On 16.05.2018 21:28, Vladislav Shpilevoy wrote: >> Hello. Thanks for the patch! See my 7 comments below. >> >> On 16/05/2018 20:14, Kirill Shcherbatov wrote: >>> Removed ?N binding, changed $V to $N semantics to match >>> other vendors standarts. >>> >>> Closes #2948 >>> --- >> >> 5. This function always must take valid variable, it is guaranteed by a parser. Please, >> do this check in parse.y. ?nnn - is syntax error. > > +++ b/src/box/sql/parse.y > @@ -897,7 +897,11 @@ expr(A) ::= VARIABLE(X). { > if( !(X.z[0]=='#' && sqlite3Isdigit(X.z[1])) ){ > u32 n = X.n; > spanExpr(&A, pParse, TK_VARIABLE, X); > - sqlite3ExprAssignVarNumber(pParse, A.pExpr, n); > + if (A.pExpr->u.zToken[0] == '?' && n > 1) { > + sqlite3ErrorMsg(pParse, "Unsupported variable format"); 1. As I said, it is syntax error, not unsupported format. > + } else { > + sqlite3ExprAssignVarNumber(pParse, A.pExpr, n); > + } > }else{ > > >> 7. I found, that :NNN works too, including SQLite. Please, remove it too. >> It works because SQLite interprets any symbols except '$' and '?' as prefix for >> name or number parameter. > > @@ -1141,6 +1139,13 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) > x = (ynVar) (++pParse->nVar); > doAdd = 1; > } > + if (n > 1 && (!sqlite3Isalpha(z[1]) || > + sqlite3CheckIdentifierName(pParse, &z[1]) != 2. Wrong alignment. > + SQLITE_OK)) { > + sqlite3ErrorMsg(pParse, > + "name '%s' is invalid identifier", z); > + return; > + } > } 3. This function takes already valid identifier. Again - check this in the parser. 4. n > 1 is guaranteed by the checks above. 5. Why do you need !sqlite3Isalpha(z[1])? '1a' is valid identifier, but your check forbids it. Strictly speaking, any number is valid identifier too. So mayby I was wrong, lets allow ':NNNN' syntax. But here NNNN will be interpreted as name, not number. It should be documented. 6. I still do not see TarantoolBot request in the issue comments.