[tarantool-patches] Re: [PATCH] sql: xfer optimization issue

  • From: Nikita Tatunov <hollow653@xxxxxxxxx>
  • To: korablev@xxxxxxxxxxxxx
  • Date: Fri, 20 Jul 2018 14:56:58 +0300

Hello, Nikita! Thank you for review!

пт, 20 июл. 2018 г. в 6:20, n.pettik <korablev@xxxxxxxxxxxxx>:

Several minor remarks.

@@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse,      /* Parser
context */

      /*
       * Xfer optimization is unable to correctly insert data
-      * in case there's a conflict action other than *_ABORT.
-      * This is the reason we want to only run it if the
-      * destination table is initially empty.
+      * in case there's a conflict action other than
+      * explicit *_ABORT. This is the reason we want to only
+      * run it if the destination table is initially empty.
       * That block generates code to make that determination.
       */
-
      if (!(onError == ON_CONFLICT_ACTION_ABORT &&
          is_err_action_default == false)) {

AFAIK we don’t use == false comparison. Instead simply use !
is_rr_action_default.


Done.


              addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5f9bc13..bc169d9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4009,7 +4009,7 @@ case OP_RowData: {
      u32 n;

 #ifdef SQLITE_TEST
-     if (pOp->p5 == 1) {
+     if (pOp->p5 == OPFLAG_XFER_OPT) {

Flags are usually tested with & operation:

if (pOp->p5 & OPFLAG_XFER_OPT != 0)


Fixed it.


              pOp->p5 = 0;
              sql_xfer_count++;
      }
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 34f603f..29f0efe 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -1,9 +1,29 @@

+local function do_xfer_test(test_number, return_code)
+     test_name = string.format("xfer-optimization-1.%d", test_number)
+     test:do_test(
+             test_name,
+             function()
+                     if (aftr - bfr == 1) then
+                             return {1}
+                     end
+                     if (aftr == bfr) then
+                             return {0}
+                     end

You can simplify it to:

return aftr - bfr;

:)


True. Fixed it.


Also

+      /* The Vdbe we're building*/
+      Vdbe *v = sqlite3GetVdbe(pParse);

Use ’struct’ modifier for complex types.
And put dot at the end of comment.


Fixed.

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 3c3bf37..4f52fa5 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser context */
  * table (tab1) is initially empty.
  */

- /* The Vdbe we're building*/
+ /* The Vdbe struct we're building. */
  Vdbe *v = sqlite3GetVdbe(pParse);
  iSrc = pParse->nTab++;
  iDest = pParse->nTab++;
@@ -1887,7 +1887,7 @@ xferOptimization(Parse * pParse, /* Parser context */
  * That block generates code to make that determination.
  */
  if (!(onError == ON_CONFLICT_ACTION_ABORT &&
-     is_err_action_default == false)) {
+     !is_err_action_default)) {
  addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
  VdbeCoverage(v);
  emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bc169d9..aa7f250 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4009,7 +4009,7 @@ case OP_RowData: {
  u32 n;

 #ifdef SQLITE_TEST
- if (pOp->p5 == OPFLAG_XFER_OPT) {
+ if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {
  pOp->p5 = 0;
  sql_xfer_count++;
  }
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 29f0efe..05d30c6 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -9,12 +9,7 @@ local function do_xfer_test(test_number, return_code)
  test:do_test(
  test_name,
  function()
- if (aftr - bfr == 1) then
- return {1}
- end
- if (aftr == bfr) then
- return {0}
- end
+ return {aftr - bfr}
  end, {
  -- <test_name>
  return_code

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018