[tarantool-patches] Re: [PATCH] sql: Remove 'BEGIN TRANSACTION'

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

Hi! Thanks for the patch! See 2 comments below.

> commit 1e031e580ca3242baa91aabf5b436ae8f3088669
> Author: N.Tatunov <hollow653@gmail.com>
> Date:   Wed Jul 4 20:21:06 2018 +0300
> 
>     sql: remove 'BEGIN TRANSACTION'
>     
>     Previously "BEGIN" / "BEGIN TRANSACTION", "COMMIT TRANSACTION" /
>     "END" / "END TRANSACTION", "ROLLBACK TRANSACTION" could be used
>     to handle transactions. By changing these commands syntax in
>     parser we're aiming on getting closer to ANSI SQL. Also found
>     initialization in ifdef that caused some problems with error
>     messages in occasions when the wrong syntax was used.
>     
>     With the patch applied only following commands can be used:
>      - "START TRANSACTION" to begin transaction.
>      - "COMMIT" to end transaction.
>      - "ROLLBACK" to rollback transaction without savepoints.
>      - "ROLLBACK TO .." to rollback transaction to savepoint.
>     
>     Closes #2164
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b1650cfa3..b26728d70 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4826,23 +4826,32 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
>  	 *       TK_EQ              OP_Ne
>  	 *       TK_GT              OP_Le
>  	 *       TK_LE              OP_Gt
> -	 *       TK_GE              OP_Lt
>  	 *       TK_LT              OP_Ge
> +	 *       TK_GE              OP_Lt

1. The table now does not match the actual values of
OP_... and TK_... values. TK_NE(13) now != OP_Eq(14),
OP_Ne < OP_Eq etc.

Speaking in general, I understand the original optimization -
it allowed to convert TK_ into OP_ not via if's, but via single
expression ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1).

The only purpose was avoid if-ing. But now we have ifs. I think,
that you should either try to do not break this dependency,
or remove it completely in a separate patch. For example,
the expression above can be inlined into switch (pExpr->op) below,
that anyway checks all TK_... value. On switch (pExpr->op)
case TK_NE you can manually set op to OP_Ne. Same for other
LT, LE, GT etc.

>  	 *        ...                ...
>  	 *       TK_ISNULL          OP_NotNull
>  	 *       TK_NOTNULL         OP_IsNull
>  	 *
> diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua
> new file mode 100755
> index 000000000..98ca531aa
> --- /dev/null
> +++ b/test/sql-tap/start-transaction.test.lua
> @@ -0,0 +1,266 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(21)

2. Please, put here the issue number and a description
in a comment.