[tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings

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

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 <v.shpilevoy@tarantool.org>
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.
>