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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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
>> ---
> 
> 1. Put the branch and issue links here please.

Branch: http://github.com/tarantool/tarantool/tree/gh-2948-unnecessary-bindings
Issue: https://github.com/tarantool/tarantool/issues/2948

> 2. I still can grep ?nnn in sqliteLimit.h.

--- a/src/box/sql/sqliteLimit.h
+++ b/src/box/sql/sqliteLimit.h
@@ -39,7 +39,7 @@
-	 * The maximum value of a ?nnn wildcard that the parser will accept.
+	 * The maximum value of a $nnn wildcard that the parser will accept.

> 3. I still see tests on $aaa here: sql-tap/e_expr.test.lua.
> 4. When you fix comments, please, align them by 66 symbols.

iff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
index 90cd15f..75f5c3c 100755
--- a/test/sql-tap/e_expr.test.lua
+++ b/test/sql-tap/e_expr.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(14749)
+test:plan(14747)
 
 --!./tcltestrunner.lua
 -- 2010 July 16
@@ -156,11 +156,11 @@ for _, op1 in ipairs(oplist) do
             local B = val[2]
             local C = val[3]
             local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
-            -- If $op2 groups more tightly than $op1, then the result
-            -- of executing $sql1 whould be the same as executing $sql3.
-            -- If $op1 groups more tightly, or if $op1 and $op2 have
-            -- the same precedence, then executing $sql1 should return
-            -- the same value as $sql2.
+            -- If ?op2 groups more tightly than ?op1, then the result
+            -- of executing ?sql1 whould be the same as executing ?sql3.
+            -- If ?op1 groups more tightly, or if ?op1 and ?op2 have
+            -- the same precedence, then executing ?sql1 should return
+            -- the same value as ?sql2.
             --
             local sql1 = string.format("SELECT %s %s %s %s %s", A, op1, B, op2, C)
             local sql2 = string.format("SELECT (%s %s %s) %s %s", A, op1, B, op2, C)
@@ -1214,55 +1214,13 @@ if (0>0) then
       SELECT @เอศขูเอล
     ]], "1 @เอศขูเอล", -1)
     parameter_test("e_expr-11.3.6", "SELECT @€", "1 @€", -1)
-    -- EVIDENCE-OF: R-62610-51329 A dollar-sign followed by an identifier
-    -- name also holds a spot for a named parameter with the name $AAAA.
-    --
-    -- EVIDENCE-OF: R-55025-21042 The identifier name in this case can
-    -- include one or more occurrences of "::" and a suffix enclosed in
-    -- "(...)" containing any text at all.
-    --
-    -- Note: Looks like an identifier cannot consist entirely of "::"
-    -- characters or just a suffix. Also, the other named variable characters
-    -- (: and @) work the same way internally. Why not just document it that way?
-    --
-    parameter_test("e_expr-11.4.1", "SELECT $AAAA", "1 $AAAA", -1)
-    parameter_test("e_expr-11.4.2", "SELECT $123", "1 $123", -1)
-    parameter_test("e_expr-11.4.3", "SELECT $__", "1 $__", -1)
-    parameter_test("e_expr-11.4.4", "SELECT $_$_", "1 $_$_", -1)
-    parameter_test("e_expr-11.4.5", [[
-      SELECT $เอศขูเอล
-    ]], "1 $เอศขูเอล", -1)
-    parameter_test("e_expr-11.4.6", "SELECT $€", "1 $€", -1)
-    parameter_test("e_expr-11.5.1", "SELECT $::::a(++--++)", "1 $::::a(++--++)", -1)
-    parameter_test("e_expr-11.5.2", "SELECT $::a()", "1 $::a()", -1)
-    parameter_test("e_expr-11.5.3", "SELECT $::1(::#$)", "1 $::1(::#$)", -1)
-    -- EVIDENCE-OF: R-11370-04520 Named parameters are also numbered. The
-    -- number assigned is one greater than the largest parameter number
-    -- already assigned.
-    --
-    -- EVIDENCE-OF: R-42620-22184 If this means the parameter would be
-    -- assigned a number greater than SQLITE_MAX_VARIABLE_NUMBER, it is an
-    -- error.
-    --
-    parameter_test("e_expr-11.6.1", "SELECT ?, @abc", "1 {} 2 @abc", "-1 -2")
-    parameter_test("e_expr-11.6.3", "SELECT $a, ?8, ?, $b, ?2, $c", [[
-      1 $a 8 ?8 9 {} 10 $b 2 ?2 11 $c
-    ]], "-1 -8 -9 -10 -2 -11")
-    for _ in X(0, "X!foreach", [=[["tn sql",[["list","1",["SELECT ?",["mvn"],", $::a"],"2",["SELECT ?",["mvn"],", ?4, @a1"],"3",["SELECT ?",[["expr",[["mvn"],"-2"]]],", :bag, @123, $x"]]]]]=]) do
-        test:do_catchsql_test(
-            "e_expr-11.7."..tn,
-            sql, {
-                1, "too many SQL variables"
-            })
-
-    end
     -- EVIDENCE-OF: R-14068-49671 Parameters that are not assigned values
     -- using sqlite3_bind() are treated as NULL.
     --
     test:do_test(
         "e_expr-11.7.1",
         function()
-            stmt = sqlite3_prepare_v2("db", " SELECT ?, :a, @b, $d ", -1)
+            stmt = sqlite3_prepare_v2("db", " SELECT ?, :a, @b, ?d ", -1)
             sqlite3_step(stmt)
             return { sqlite3_column_type(stmt, 0), sqlite3_column_type(stmt, 1), sqlite3_column_type(stmt, 2), sqlite3_column_type(stmt, 3) }
         end, {
@@ -1506,7 +1464,6 @@ local test_cases12 ={
 
     {49, "CAST ( EXPR AS integer )"},
     {50, "CAST ( EXPR AS 'abcd' )"},
-    {51, "CAST ( EXPR AS 'ab$ $cd' )"},
 
     {52, "EXPR COLLATE \"unicode_ci\""},
     {53, "EXPR COLLATE binary"},
@@ -1634,20 +1591,6 @@ for _, val in ipairs(test_cases12) do
 
     end
 end
--- # -- syntax diagram raise-function
--- #
--- foreach {tn raiseexpr} {
---   1 "RAISE(IGNORE)"
---   2 "RAISE(ROLLBACK, 'error message')"
---   3 "RAISE(ABORT, 'error message')"
---   4 "RAISE(FAIL, 'error message')"
--- } {
---   do_execsql_test e_expr-12.4.$tn "
---     CREATE TRIGGER dbname.tr$tn BEFORE DELETE ON tblname BEGIN
---       SELECT $raiseexpr ;
---     END;
---   " {}
--- }
 ---------------------------------------------------------------------------
 -- Test the statements related to the BETWEEN operator.
 --


@@ -1067,20 +1067,20 @@ sqlite3ExprFunction(Parse * pParse, ExprList * pList, Token * pToken)
 }
 
 /*
- * Assign a variable number to an expression that encodes a wildcard
- * in the original SQL statement.
+ * Assign a variable number to an expression that encodes a
+ * wildcard in the original SQL statement.
  *
- * Wildcards consisting of a single "?" are assigned the next sequential
- * variable number.
+ * Wildcards consisting of a single "?" are assigned the next
+ * sequential variable number.
  *
- * Wildcards of the form "$nnn" are assigned the number "nnn".  We make
- * sure "nnn" is not too big to avoid a denial of service attack when
- * the SQL statement comes from an external source.
+ * Wildcards of the form "$nnn" are assigned the number "nnn".
+ * We make sure "nnn" is not too big to avoid a denial of service
+ * attack when the SQL statement comes from an external source.
  *
- * Wildcards of the form ":aaa", "@aaa", are assigned the same number
- * as the previous instance of the same wildcard.  Or if this is the first
- * instance of the wildcard, the next sequential variable number is
- * assigned.
+ * Wildcards of the form ":aaa", "@aaa", are assigned the same
+ * number as the previous instance of the same wildcard.  Or if
+ * this is the first instance of the wildcard, the next sequential variable
+ * number is assigned.
  */
> 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");
+    } else {
+        sqlite3ExprAssignVarNumber(pParse, A.pExpr, n);
+    }
   }else{

+++ b/src/box/sql/expr.c
@@ -1103,10 +1103,8 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 		x = (ynVar) (++pParse->nVar);
 	} else {
 		int doAdd = 0;
-		if (z[0] == '?') {
-			sqlite3ErrorMsg(pParse, "Unsupported variable format");
-			return;
-		} else if (z[0] == '$') {
+		assert(z[0] != '?');
+		if (z[0] == '$') {

> 6. Error message in this 'if' body still outputs '?NNN'
@@ -1120,7 +1118,7 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 			testcase(i == SQL_BIND_PARAMETER_MAX);
 			if (bOk == 0 || i < 1 || i > SQL_BIND_PARAMETER_MAX) {
 				sqlite3ErrorMsg(pParse,
-						"variable number must be between ?1 and ?%d",
+						"variable number must be between $1 and $%d",
 						SQL_BIND_PARAMETER_MAX);
 
> 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]) !=
+			     SQLITE_OK)) {
+				sqlite3ErrorMsg(pParse,
+						"name '%s' is invalid identifier", z);
+				return;
+			}
 		}

--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3960,7 +3960,7 @@ Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int);
Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *);
Expr *sqlite3ExprSkipCollate(Expr *);
int sqlite3CheckCollSeq(Parse *, struct coll *);
-int sqlite3CheckIdentifierName(Parse *, char *);
+int sqlite3CheckIdentifierName(Parse *, const char *);


diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 4305eb9..7be4470 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -447,6 +447,10 @@ cn:execute('select $2, $1, $3', parameters)
   rows:
   - [22, 11, 33]
 ...
+cn:execute('select * from test where id = :1', {1})
+---
+- error: 'Failed to execute SQL statement: name '':1'' is invalid identifier'
+...
 -- gh-2602 obuf_alloc breaks the tuple in different slabs
 _ = space:replace{1, 1, string.rep('a', 4 * 1024 * 1024)}
 ---
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index cbeaddd..c7e9695 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -170,6 +170,7 @@ parameters[1] = 11
 parameters[2] = 22
 parameters[3] = 33
 cn:execute('select $2, $1, $3', parameters)
+cn:execute('select * from test where id = :1', {1})
 
 -- gh-2602 obuf_alloc breaks the tuple in different slabs
 _ = space:replace{1, 1, string.rep('a', 4 * 1024 * 1024)}