Hello, Nikita! Thank you for review!

пт, 20 июл. 2018 г. в 6:20, n.pettik <korablev@tarantool.org>:
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