[tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 24 15:20:17 MSK 2018
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 at 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.
>
More information about the Tarantool-patches
mailing list