<div dir="ltr"><br>Hello, Nikita! Thank you for review!<br><br><div class="gmail_quote"><div dir="ltr">пт, 20 июл. 2018 г. в 6:20, n.pettik <<a href="mailto:korablev@tarantool.org">korablev@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">Several minor remarks.<br>
<br>
> @@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse, /* Parser context */<br>
> <br>
> /*<br>
> * Xfer optimization is unable to correctly insert data<br>
> - * in case there's a conflict action other than *_ABORT.<br>
> - * This is the reason we want to only run it if the<br>
> - * destination table is initially empty.<br>
> + * in case there's a conflict action other than<br>
> + * explicit *_ABORT. This is the reason we want to only<br>
> + * run it if the destination table is initially empty.<br>
> * That block generates code to make that determination.<br>
> */<br>
> -<br>
> if (!(onError == ON_CONFLICT_ACTION_ABORT &&<br>
> is_err_action_default == false)) {<br>
<br>
AFAIK we don’t use == false comparison. Instead simply use ! is_rr_action_default.<br>
<br></blockquote><div><br></div><div>Done.</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">
> addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);<br>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c<br>
> index 5f9bc13..bc169d9 100644<br>
> --- a/src/box/sql/vdbe.c<br>
> +++ b/src/box/sql/vdbe.c<br>
> @@ -4009,7 +4009,7 @@ case OP_RowData: {<br>
> u32 n;<br>
> <br>
> #ifdef SQLITE_TEST<br>
> - if (pOp->p5 == 1) {<br>
> + if (pOp->p5 == OPFLAG_XFER_OPT) {<br>
<br>
Flags are usually tested with & operation:<br>
<br>
if (pOp->p5 & OPFLAG_XFER_OPT != 0)<br>
<br></blockquote><div><br></div><div>Fixed it.</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">
> pOp->p5 = 0;<br>
> sql_xfer_count++;<br>
> }<br>
> diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua<br>
> index 34f603f..29f0efe 100755<br>
> --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua<br>
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua<br>
> @@ -1,9 +1,29 @@<br>
> <br>
> +local function do_xfer_test(test_number, return_code)<br>
> + test_name = string.format("xfer-optimization-1.%d", test_number)<br>
> + test:do_test(<br>
> + test_name,<br>
> + function()<br>
> + if (aftr - bfr == 1) then<br>
> + return {1}<br>
> + end<br>
> + if (aftr == bfr) then<br>
> + return {0}<br>
> + end<br>
<br>
You can simplify it to:<br>
<br>
return aftr - bfr;<br>
<br>
:)<br>
<br></blockquote><div><br></div><div>True. Fixed it.</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">
Also<br>
<br>
>+ /* The Vdbe we're building*/<br>
>+ Vdbe *v = sqlite3GetVdbe(pParse);<br>
<br>
Use ’struct’ modifier for complex types.<br>
And put dot at the end of comment.<br>
<br></blockquote><div><br></div><div>Fixed. </div><div><br></div><div><div>diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c</div><div>index 3c3bf37..4f52fa5 100644</div><div>--- a/src/box/sql/insert.c</div><div>+++ b/src/box/sql/insert.c</div><div>@@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse,<span style="white-space:pre"> </span>/* Parser context */</div><div> <span style="white-space:pre"> </span> * table (tab1) is initially empty.</div><div> <span style="white-space:pre"> </span> */</div><div> </div><div>-<span style="white-space:pre"> </span>/* The Vdbe we're building*/</div><div>+<span style="white-space:pre"> </span>/* The Vdbe struct we're building. */</div><div> <span style="white-space:pre"> </span>Vdbe *v = sqlite3GetVdbe(pParse);</div><div> <span style="white-space:pre"> </span>iSrc = pParse->nTab++;</div><div> <span style="white-space:pre"> </span>iDest = pParse->nTab++;</div><div>@@ -1887,7 +1887,7 @@ xferOptimization(Parse * pParse,<span style="white-space:pre"> </span>/* Parser context */</div><div> <span style="white-space:pre"> </span> * That block generates code to make that determination.</div><div> <span style="white-space:pre"> </span> */</div><div> <span style="white-space:pre"> </span>if (!(onError == ON_CONFLICT_ACTION_ABORT &&</div><div>-<span style="white-space:pre"> </span> is_err_action_default == false)) {</div><div>+<span style="white-space:pre"> </span> !is_err_action_default)) {</div><div> <span style="white-space:pre"> </span>addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);</div><div> <span style="white-space:pre"> </span>VdbeCoverage(v);</div><div> <span style="white-space:pre"> </span>emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);</div><div>diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c</div><div>index bc169d9..aa7f250 100644</div><div>--- a/src/box/sql/vdbe.c</div><div>+++ b/src/box/sql/vdbe.c</div><div>@@ -4009,7 +4009,7 @@ case OP_RowData: {</div><div> <span style="white-space:pre"> </span>u32 n;</div><div> </div><div> #ifdef SQLITE_TEST</div><div>-<span style="white-space:pre"> </span>if (pOp->p5 == OPFLAG_XFER_OPT) {</div><div>+<span style="white-space:pre"> </span>if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {</div><div> <span style="white-space:pre"> </span>pOp->p5 = 0;</div><div> <span style="white-space:pre"> </span>sql_xfer_count++;</div><div> <span style="white-space:pre"> </span>}</div><div>diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua</div><div>index 29f0efe..05d30c6 100755</div><div>--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua</div><div>+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua</div><div>@@ -9,12 +9,7 @@ local function do_xfer_test(test_number, return_code)</div><div> <span style="white-space:pre"> </span>test:do_test(</div><div> <span style="white-space:pre"> </span>test_name,</div><div> <span style="white-space:pre"> </span>function()</div><div>-<span style="white-space:pre"> </span>if (aftr - bfr == 1) then</div><div>-<span style="white-space:pre"> </span>return {1}</div><div>-<span style="white-space:pre"> </span>end</div><div>-<span style="white-space:pre"> </span>if (aftr == bfr) then</div><div>-<span style="white-space:pre"> </span>return {0}</div><div>-<span style="white-space:pre"> </span>end</div><div>+<span style="white-space:pre"> </span>return {aftr - bfr}</div><div> <span style="white-space:pre"> </span>end, {</div><div> <span style="white-space:pre"> </span>-- <test_name></div><div> <span style="white-space:pre"> </span>return_code</div></div><div><br></div></div></div>