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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Removed ?N binding, changed $V to $N semantics to match
other vendors standarts.

Closes #2948
---
 src/box/sql/expr.c           | 13 +++++++-----
 test/sql-tap/e_expr.test.lua | 50 +++-----------------------------------------
 test/sql/iproto.result       | 33 +++++++++++++++++++++++++----
 test/sql/iproto.test.lua     | 14 +++++++++++--
 4 files changed, 52 insertions(+), 58 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 1b51823..cd2f549 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1073,11 +1073,11 @@ sqlite3ExprFunction(Parse * pParse, ExprList * pList, Token * pToken)
  * Wildcards consisting of a single "?" are assigned the next sequential
  * variable number.
  *
- * Wildcards of the form "?nnn" are assigned the number "nnn".  We make
+ * 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", or "$aaa" are assigned the same number
+ * 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.
@@ -1104,7 +1104,10 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 	} else {
 		int doAdd = 0;
 		if (z[0] == '?') {
-			/* Wildcard of the form "?nnn".  Convert "nnn" to an integer and
+			sqlite3ErrorMsg(pParse, "Unsupported variable format");
+			return;
+		} else if (z[0] == '$') {
+			/* Wildcard of the form "$nnn".  Convert "nnn" to an integer and
 			 * use it as the variable number
 			 */
 			i64 i;
@@ -1129,7 +1132,7 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
 				doAdd = 1;
 			}
 		} else {
-			/* Wildcards like ":aaa", "$aaa" or "@aaa".  Reuse the same variable
+			/* Wildcards like ":aaa", or "@aaa".  Reuse the same variable
 			 * number as the prior appearance of the same name, or if the name
 			 * has never appeared before, reuse the same variable number
 			 */
@@ -3828,7 +3831,7 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				const char *z =
 				    sqlite3VListNumToName(pParse->pVList,
 							  pExpr->iColumn);
-				assert(pExpr->u.zToken[0] == '?'
+				assert(pExpr->u.zToken[0] == '$'
 				       || strcmp(pExpr->u.zToken, z) == 0);
 				pParse->pVList[0] = 0;	/* Indicate VList may no longer be enlarged */
 				sqlite3VdbeAppendP4(v, (char *)z, P4_STATIC);
diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
index d0f6895..90cd15f 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(14750)
+test:plan(14749)
 
 --!./tcltestrunner.lua
 -- 2010 July 16
@@ -1188,48 +1188,6 @@ if (0>0) then
         X(492, "X!cmd", [=[["do_test",[["tn"],".res"],[["list","set","",["res"]]],["result"]]]=])
     end
 
-    -- EVIDENCE-OF: R-33509-39458 A question mark followed by a number NNN
-    -- holds a spot for the NNN-th parameter. NNN must be between 1 and
-    -- SQLITE_MAX_VARIABLE_NUMBER.
-    --
-    mvn = SQLITE_MAX_VARIABLE_NUMBER
-    parameter_test("e_expr-11.1", string.format([[
-      SELECT ?1, ?123, ?%s, ?123, ?4
-    ]], SQLITE_MAX_VARIABLE_NUMBER), string.format("1 ?1  123 ?123 %s ?%s 4 ?4", mvn, mvn), "-1 -123 -"..mvn.." -123 -4")
-    errmsg = "variable number must be between ?1 and ?"..SQLITE_MAX_VARIABLE_NUMBER..""
-    for _ in X(0, "X!foreach", [=[["tn param_number",[["list","2","0","3",[["expr",[["SQLITE_MAX_VARIABLE_NUMBER"],"+1"]]],"4",[["expr",[["SQLITE_MAX_VARIABLE_NUMBER"],"+2"]]],"5","12345678903456789034567890234567890","6","2147483648","7","2147483649","8","4294967296","9","4294967297","10","9223372036854775808","11","9223372036854775809","12","18446744073709551616","13","18446744073709551617"]]]]=]) do
-        test:do_catchsql_test(
-            "e_expr-11.1."..tn,
-            "SELECT ?"..param_number.."", {
-                1, errmsg
-            })
-
-    end
-    -- EVIDENCE-OF: R-33670-36097 A question mark that is not followed by a
-    -- number creates a parameter with a number one greater than the largest
-    -- parameter number already assigned.
-    --
-    -- EVIDENCE-OF: R-42938-07030 If this means the parameter number is
-    -- greater than SQLITE_MAX_VARIABLE_NUMBER, it is an error.
-    --
-    parameter_test("e_expr-11.2.1", "SELECT ?", "1 {}", -1)
-    parameter_test("e_expr-11.2.2", "SELECT ?, ?", "1 {} 2 {}", "-1 -2")
-    parameter_test("e_expr-11.2.3", "SELECT ?5, ?", "5 ?5 6 {}", "-5 -6")
-    parameter_test("e_expr-11.2.4", "SELECT ?, ?5", "1 {} 5 ?5", "-1 -5")
-    parameter_test("e_expr-11.2.5", "SELECT ?, ?456, ?", [[
-      1 {} 456 ?456 457 {}
-    ]], "-1 -456 -457")
-    parameter_test("e_expr-11.2.5", "SELECT ?, ?456, ?4, ?", [[
-      1 {} 456 ?456 4 ?4 457 {}
-    ]], "-1 -456 -4 -457")
-    for _ in X(0, "X!foreach", [=[["tn sql",[["list","1",["SELECT ?",["mvn"],", ?"],"2",["SELECT ?",[["expr",[["mvn"],"-5"]]],", ?, ?, ?, ?, ?, ?"],"3",["SELECT ?",[["expr",["mvn"]]],", ?5, ?6, ?"]]]]]=]) do
-        test:do_catchsql_test(
-            "e_expr-11.3."..tn,
-            sql, {
-                1, "too many SQL variables"
-            })
-
-    end
     -- EVIDENCE-OF: R-11620-22743 A colon followed by an identifier name
     -- holds a spot for a named parameter with the name :AAAA.
     --
@@ -1287,7 +1245,6 @@ if (0>0) then
     -- error.
     --
     parameter_test("e_expr-11.6.1", "SELECT ?, @abc", "1 {} 2 @abc", "-1 -2")
-    parameter_test("e_expr-11.6.2", "SELECT ?123, :a1", "123 ?123 124 :a1", "-123 -124")
     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")
@@ -1503,11 +1460,10 @@ local test_cases12 ={
     {8, "CURRENT_TIMESTAMP"},
 
     {9, "?"},
-    {10, "?123"},
     {11, "@hello"},
     {12, ":world"},
-    {13, "$tcl"},
-    {14, "$tcl(array)"},
+    {13, ":tcl"},
+    {14, ":tcl(array)"},
 
     {15, "cname"},
     {16, "tblname.cname"},
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index b251c80..4305eb9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -164,7 +164,7 @@ parameters = {}
 parameters[1] = {}
 ---
 ...
-parameters[1]['$value3'] = 1
+parameters[1][':value3'] = 1
 ---
 ...
 parameters[2] = 2
@@ -188,10 +188,10 @@ parameters[6] = {}
 parameters[6]['@value2'] = 6
 ---
 ...
-cn:execute('select $value3, ?, :value1, ?, ?, @value2, ?, $value3', parameters)
+cn:execute('select :value3, ?, :value1, ?, ?, @value2, ?, :value3', parameters)
 ---
-- metadata: [{'name': $value3}, {'name': '?'}, {'name': ':value1'}, {'name': '?'},
-    {'name': '?'}, {'name': '@value2'}, {'name': '?'}, {'name': '$value3'}]
+- metadata: [{'name': ':value3'}, {'name': '?'}, {'name': ':value1'}, {'name': '?'},
+    {'name': '?'}, {'name': '@value2'}, {'name': '?'}, {'name': ':value3'}]
   rows:
   - [1, 2, 3, 4, 5, 6, null, 1]
 ...
@@ -422,6 +422,31 @@ cn:execute('drop table if exists test3')
 ---
 - rowcount: 0
 ...
+--
+-- gh-2948: sql: remove unnecessary templates for binding parameters
+--
+cn:execute('select ?1, ?2, ?3', {1, 2, 3})
+---
+- error: 'Failed to execute SQL statement: Unsupported variable format'
+...
+parameters = {}
+---
+...
+parameters[1] = 11
+---
+...
+parameters[2] = 22
+---
+...
+parameters[3] = 33
+---
+...
+cn:execute('select $2, $1, $3', parameters)
+---
+- metadata: [{'name': $2}, {'name': '$1'}, {'name': '$3'}]
+  rows:
+  - [22, 11, 33]
+...
 -- 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 9d7c825..cbeaddd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -60,7 +60,7 @@ cn:execute('select ?, :value1, @value2', parameters)
 
 parameters = {}
 parameters[1] = {}
-parameters[1]['$value3'] = 1
+parameters[1][':value3'] = 1
 parameters[2] = 2
 parameters[3] = {}
 parameters[3][':value1'] = 3
@@ -68,7 +68,7 @@ parameters[4] = 4
 parameters[5] = 5
 parameters[6] = {}
 parameters[6]['@value2'] = 6
-cn:execute('select $value3, ?, :value1, ?, ?, @value2, ?, $value3', parameters)
+cn:execute('select :value3, ?, :value1, ?, ?, @value2, ?, :value3', parameters)
 
 -- Try not-integer types.
 msgpack = require('msgpack')
@@ -161,6 +161,16 @@ cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)')
 cn:execute('drop table test3')
 cn:execute('drop table if exists test3')
 
+--
+-- gh-2948: sql: remove unnecessary templates for binding parameters
+--
+cn:execute('select ?1, ?2, ?3', {1, 2, 3})
+parameters = {}
+parameters[1] = 11
+parameters[2] = 22
+parameters[3] = 33
+cn:execute('select $2, $1, $3', parameters)
+
 -- gh-2602 obuf_alloc breaks the tuple in different slabs
 _ = space:replace{1, 1, string.rep('a', 4 * 1024 * 1024)}
 res = cn:execute('select * from test')
-- 
2.7.4