<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">чт, 19 июл. 2018 г. в 18:44, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi! Thanks for the patch! See 2 comments below.<br>
<br>
> commit 1e031e580ca3242baa91aabf5b436ae8f3088669<br>
> Author: N.Tatunov <<a href="mailto:hollow653@gmail.com" target="_blank">hollow653@gmail.com</a>><br>
> Date:   Wed Jul 4 20:21:06 2018 +0300<br>
> <br>
>     sql: remove 'BEGIN TRANSACTION'<br>
>     <br>
>     Previously "BEGIN" / "BEGIN TRANSACTION", "COMMIT TRANSACTION" /<br>
>     "END" / "END TRANSACTION", "ROLLBACK TRANSACTION" could be used<br>
>     to handle transactions. By changing these commands syntax in<br>
>     parser we're aiming on getting closer to ANSI SQL. Also found<br>
>     initialization in ifdef that caused some problems with error<br>
>     messages in occasions when the wrong syntax was used.<br>
>     <br>
>     With the patch applied only following commands can be used:<br>
>      - "START TRANSACTION" to begin transaction.<br>
>      - "COMMIT" to end transaction.<br>
>      - "ROLLBACK" to rollback transaction without savepoints.<br>
>      - "ROLLBACK TO .." to rollback transaction to savepoint.<br>
>     <br>
>     Closes #2164<br>
> <br>
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c<br>
> index b1650cfa3..b26728d70 100644<br>
> --- a/src/box/sql/expr.c<br>
> +++ b/src/box/sql/expr.c<br>
> @@ -4826,23 +4826,32 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)<br>
>        *       TK_EQ              OP_Ne<br>
>        *       TK_GT              OP_Le<br>
>        *       TK_LE              OP_Gt<br>
> -      *       TK_GE              OP_Lt<br>
>        *       TK_LT              OP_Ge<br>
> +      *       TK_GE              OP_Lt<br>
<br>
1. The table now does not match the actual values of<br>
OP_... and TK_... values. TK_NE(13) now != OP_Eq(14),<br>
OP_Ne < OP_Eq etc.<br>
<br>
Speaking in general, I understand the original optimization -<br>
it allowed to convert TK_ into OP_ not via if's, but via single<br>
expression ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1).<br>
<br>
The only purpose was avoid if-ing. But now we have ifs. I think,<br>
that you should either try to do not break this dependency,<br>
or remove it completely in a separate patch. For example,<br>
the expression above can be inlined into switch (pExpr->op) below,<br>
that anyway checks all TK_... value. On switch (pExpr->op)<br>
case TK_NE you can manually set op to OP_Ne. Same for other<br>
LT, LE, GT etc.<br>
<br></blockquote><div><br></div><div>The table only shows the relation between pExpr->op and op.</div><div>It shouldn't match mapping between OP_* and TK_* values</div><div>indicating the same operation.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>        *        ...                ...<br>
>        *       TK_ISNULL          OP_NotNull<br>
>        *       TK_NOTNULL         OP_IsNull<br>
>        *<br>
> diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua<br>
> new file mode 100755<br>
> index 000000000..98ca531aa<br>
> --- /dev/null<br>
> +++ b/test/sql-tap/start-transaction.test.lua<br>
> @@ -0,0 +1,266 @@<br>
> +#!/usr/bin/env tarantool<br>
> +test = require("sqltester")<br>
> +test:plan(21)<br>
<br>
2. Please, put here the issue number and a description<br>
in a comment.<br></blockquote><div><br></div><div>Done.</div><div><br></div><div>Diff for changes in this patch:<br><br></div><div>diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua</div><div>index 98ca531..82356ae 100755</div><div>--- a/test/sql-tap/start-transaction.test.lua</div><div>+++ b/test/sql-tap/start-transaction.test.lua</div><div>@@ -2,6 +2,14 @@</div><div> test = require("sqltester")</div><div> test:plan(21)</div><div> </div><div>+-----------------------------------------------------------------</div><div>+-- Previously "BEGIN" / "BEGIN TRANSACTION", "COMMIT TRANSACTION"</div><div>+-- / "END" / "END TRANSACTION", "ROLLBACK TRANSACTION" could be</div><div>+-- used to handle transactions. By changing these commands syntax</div><div>+-- in parser we're aiming on getting closer to ANSI SQL.</div><div>+-----------------------------------------------------------------</div><div>+-- Tarantool issue: #2164.</div><div>+</div><div> test:do_catchsql_test(</div><div> <span style="white-space:pre">  </span>"start-transaction-1.0",</div><div> <span style="white-space:pre">  </span>[[ </div></div></div>