[tarantool-patches] Re: [PATCH] sql: refactor SQL cursor to remove write ones

[Kirill Yukhin]

Hello,

On 04 окт 13:45, n.pettik wrote:
> > Hello Nikita!
> > Thanks for review. All nits applied.
> > Updated patch in the bottom. Branch force-pushed.
> 
> IDK whether it is outlined in our review policy or not, but it is quite
> hard to review each time the whole patch again. So, it would be nice
> if you attached intermediate diff (i.e. diff between two versions of patch)
> in order to review only parts which have changed. Or, inline diff for
> each comment/remark.
Here it is:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 929d3c8..d28b3f1 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1005,7 +1005,8 @@ selectInnerLoop(Parse * pParse,           /* The parser context */
                                regPrev = pParse->nMem + 1;
                                pParse->nMem += nResultCol;

-                           /* Actually, for DISTINCT handling
+                         /*
+                          * Actually, for DISTINCT handling
                                 * two op-codes were emitted here:
                                 * OpenTEphemeral & IteratorOpen.
                                 * So, we need to Noop one and
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d28b3f1..b4f7a2b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -86,7 +86,7 @@ struct SortCtx {
        ExprList *pOrderBy;     /* The ORDER BY (or GROUP BY clause) */
        int nOBSat;             /* Number of ORDER BY terms satisfied by indices */
        int iECursor;           /* Cursor number for the sorter */
-   /* Register, containing pointer to ephemeral space. */
+ /** Register, containing pointer to ephemeral space. */
        int reg_eph;
        int regReturn;          /* Register holding block-output return address */
        int labelBkOut;         /* Start label for the block-output subroutine */
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 352f2ae..49aef33 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3367,7 +3367,7 @@ void sqlite3EndTable(Parse *, Token *, Select *);
 /**
  * Create cursor which will be positioned to the space/index.
  * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
+ * which is passes to OP_IteratorOpen as an argument.
  *
  * @param parse_context Parse context.
  * @param cursor Number of cursor to be created.

> >>>> Finally, now I think about renaming CursorOpen to IteratorOpen: in fact after your
> >>>> patch cursor has turned almost in wrapper around index iterator. Such naming
> >>>> would underline the fact that ‘cursor’ is used only for read iterations over the space.
> >>> Yes, sure. Renamed.
> >> 
> >> You forgot to update name of opcode in commit message.
> >> Also, you deleted not all mentions of OP_Cursor(Re)Open - just grep it
> 
> Still, I can grep OP_OpenWrite. Eliminate it completely pls.
Fixed.

> Consider code style fixes:
Added.

I'll commit it to 2.0 branch today, thanks!

--
Regards, Kirill Yukhin