* [tarantool-patches] [PATCH v1 1/1] sql: remove unnecessary templates for bindings
@ 2018-05-16 17:14 Kirill Shcherbatov
2018-05-16 18:28 ` [tarantool-patches] " Vladislav Shpilevoy
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-05-16 17:14 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-16 17:14 [tarantool-patches] [PATCH v1 1/1] sql: remove unnecessary templates for bindings Kirill Shcherbatov
@ 2018-05-16 18:28 ` Vladislav Shpilevoy
2018-05-17 10:39 ` Kirill Shcherbatov
2018-05-17 14:20 ` Konstantin Osipov
2018-05-18 5:55 ` Konstantin Osipov
2 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-05-16 18:28 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
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.
> 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
2. I still can grep ?nnn in sqliteLimit.h.
> * 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
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.
> @@ -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");
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.
> + return;
> + } else if (z[0] == '$') {
6. Error message in this 'if' body still outputs '?NNN'
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.
Example:
tarantool> cn:execute('select * from test where id = :1', {1})
---
- metadata: [{'name': ID}, {'name': 'A'}, {'name': 'B'}]
rows:
- [1, 2, '3']
...
We must forbid it.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-16 18:28 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-05-17 10:39 ` Kirill Shcherbatov
2018-05-17 11:56 ` Vladislav Shpilevoy
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-05-17 10:39 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
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)}
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-17 10:39 ` Kirill Shcherbatov
@ 2018-05-17 11:56 ` Vladislav Shpilevoy
2018-05-23 15:42 ` Kirill Shcherbatov
0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-05-17 11:56 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-16 17:14 [tarantool-patches] [PATCH v1 1/1] sql: remove unnecessary templates for bindings Kirill Shcherbatov
2018-05-16 18:28 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-05-17 14:20 ` Konstantin Osipov
2018-05-18 5:55 ` Konstantin Osipov
2 siblings, 0 replies; 10+ messages in thread
From: Konstantin Osipov @ 2018-05-17 14:20 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/16 20:15]:
> Removed ?N binding, changed $V to $N semantics to match
> other vendors standarts.
Standards.
Please take time to set up automatic spell checking in your editor.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-16 17:14 [tarantool-patches] [PATCH v1 1/1] sql: remove unnecessary templates for bindings Kirill Shcherbatov
2018-05-16 18:28 ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-17 14:20 ` Konstantin Osipov
@ 2018-05-18 5:55 ` Konstantin Osipov
2 siblings, 0 replies; 10+ messages in thread
From: Konstantin Osipov @ 2018-05-18 5:55 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/16 20:15]:
> Removed ?N binding, changed $V to $N semantics to match
> other vendors standarts.
Please make sure to open a documentation request for your changes.
>
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-17 11:56 ` Vladislav Shpilevoy
@ 2018-05-23 15:42 ` Kirill Shcherbatov
2018-05-24 12:20 ` Vladislav Shpilevoy
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-05-23 15:42 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-23 15:42 ` Kirill Shcherbatov
@ 2018-05-24 12:20 ` Vladislav Shpilevoy
2018-05-24 12:37 ` Kirill Shcherbatov
0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-05-24 12:20 UTC (permalink / raw)
To: tarantool-patches, 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 <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.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-24 12:20 ` Vladislav Shpilevoy
@ 2018-05-24 12:37 ` Kirill Shcherbatov
2018-05-24 14:51 ` Kirill Yukhin
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-05-24 12:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
> 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.
Thanks for review!
Changes squashed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
2018-05-24 12:37 ` Kirill Shcherbatov
@ 2018-05-24 14:51 ` Kirill Yukhin
0 siblings, 0 replies; 10+ messages in thread
From: Kirill Yukhin @ 2018-05-24 14:51 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
Hello,
On 24 мая 15:37, Kirill Shcherbatov wrote:
> > 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.
> Thanks for review!
> Changes squashed.
I've committed you patch to 2.0 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-24 14:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-16 17:14 [tarantool-patches] [PATCH v1 1/1] sql: remove unnecessary templates for bindings Kirill Shcherbatov
2018-05-16 18:28 ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-17 10:39 ` Kirill Shcherbatov
2018-05-17 11:56 ` Vladislav Shpilevoy
2018-05-23 15:42 ` Kirill Shcherbatov
2018-05-24 12:20 ` Vladislav Shpilevoy
2018-05-24 12:37 ` Kirill Shcherbatov
2018-05-24 14:51 ` Kirill Yukhin
2018-05-17 14:20 ` Konstantin Osipov
2018-05-18 5:55 ` Konstantin Osipov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox