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 1CCAC25006 for ; Thu, 24 May 2018 08:20:21 -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 hDjCW2brzwH5 for ; Thu, 24 May 2018 08:20:21 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 7B9592500E for ; Thu, 24 May 2018 08:20:20 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings References: <83d1862c-47af-118e-11cd-7c8ce786f468@tarantool.org> <5c5e8260-38d6-1ae9-eb01-af25124b4dc5@tarantool.org> <061016d3-9a43-35f2-6989-8ab2742a99f3@tarantool.org> From: Vladislav Shpilevoy Message-ID: <020ef0ad-0a10-e955-c737-e1766d43bd23@tarantool.org> Date: Thu, 24 May 2018 15:20:17 +0300 MIME-Version: 1.0 In-Reply-To: <061016d3-9a43-35f2-6989-8ab2742a99f3@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: tarantool-patches@freelists.org, Kirill Shcherbatov Hello. Thanks for the patch! I pushed my review fixes on the branch. Now the patch LGTM. You may squash my fixes if you want. commit da7816320606bcdc2cc247b2b7ccb9ac0c6d0af3 Author: Vladislav Shpilevoy Date: Thu May 24 15:18:34 2018 +0300 Review fixes diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 263d940ce..8a163c87b 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -1091,8 +1091,10 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) int doAdd = 0; assert(z[0] != '?'); if (z[0] == '$') { - /* Wildcard of the form "$nnn". Convert "nnn" to an integer and - * use it as the variable number + /* + * Wildcard of the form "$nnn". Convert + * "nnn" to an integer and use it as the + * variable number */ i64 i; int bOk = diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 9f6eb0c98..e71bde848 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -867,20 +867,18 @@ term(A) ::= INTEGER(X). { if( A.pExpr ) A.pExpr->flags |= EP_Leaf; } expr(A) ::= VARIABLE(X). { + Token t = X; if( !(X.z[0]=='#' && sqlite3Isdigit(X.z[1])) ){ u32 n = X.n; - Token t = X; spanExpr(&A, pParse, TK_VARIABLE, X); - if (A.pExpr->u.zToken[0] == '?' && n > 1) { + if (A.pExpr->u.zToken[0] == '?' && n > 1) sqlite3ErrorMsg(pParse, "near \"%T\": syntax error", &t); - } else { + else sqlite3ExprAssignVarNumber(pParse, A.pExpr, n); - } }else{ /* When doing a nested parse, one can include terms in an expression ** that look like this: #1 #2 ... These terms refer to registers ** in the virtual machine. #N is the N-th register. */ - Token t = X; /*A-overwrites-X*/ assert( t.n>=2 ); spanSet(&A, &t, &t); if( pParse->nested==0 ){ diff --git a/test/sql/iproto.result b/test/sql/iproto.result index 5b22838bf..3b7574ae0 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -457,12 +457,18 @@ cn:execute('drop table if exists test3') - rowcount: 0 ... -- --- gh-2948: sql: remove unnecessary templates for binding parameters +-- gh-2948: sql: remove unnecessary templates for binding +-- parameters. -- cn:execute('select ?1, ?2, ?3', {1, 2, 3}) --- - error: 'Failed to execute SQL statement: near "?1": syntax error' ... +cn:execute('select $name, $name2', {1, 2}) +--- +- error: 'Failed to execute SQL statement: variable number must be between $1 and + $65000' +... parameters = {} --- ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index c7e969528..159a8394e 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -162,9 +162,11 @@ cn:execute('drop table test3') cn:execute('drop table if exists test3') -- --- gh-2948: sql: remove unnecessary templates for binding parameters +-- gh-2948: sql: remove unnecessary templates for binding +-- parameters. -- cn:execute('select ?1, ?2, ?3', {1, 2, 3}) +cn:execute('select $name, $name2', {1, 2}) parameters = {} parameters[1] = 11 parameters[2] = 22 On 23/05/2018 18:42, Kirill Shcherbatov wrote: >> 1. As I said, it is syntax error, not unsupported format. > +++ b/src/box/sql/parse.y > @@ -896,9 +896,10 @@ term(A) ::= INTEGER(X). { > expr(A) ::= VARIABLE(X). { > if( !(X.z[0]=='#' && sqlite3Isdigit(X.z[1])) ){ > u32 n = X.n; > + Token t = X; > spanExpr(&A, pParse, TK_VARIABLE, X); > if (A.pExpr->u.zToken[0] == '?' && n > 1) { > - sqlite3ErrorMsg(pParse, "Unsupported variable format"); > + sqlite3ErrorMsg(pParse, "near \"%T\": syntax error", &t); > >> 2. Wrong alignment. >> 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. > +++ b/src/box/sql/expr.c > @@ -1139,13 +1139,6 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) > x = (ynVar) (++pParse->nVar); > doAdd = 1; > } > - if (n > 1 && (!sqlite3Isalpha(z[1]) || > - sqlite3CheckIdentifierName(pParse, &z[1]) != > - SQLITE_OK)) { > - sqlite3ErrorMsg(pParse, > - "name '%s' is invalid identifier", z); > - return; > - } > >> 6. I still do not see TarantoolBot request in the issue comments. > > done. >