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

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

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.