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

[Nikita Tatunov]

Hello, Nikita! Thank you for review!

пт, 20 июл. 2018 г. в 6:20, n.pettik <korablev at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180720/8d10f331/attachment.html>