From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A93422A872 for ; Thu, 4 Oct 2018 07:48:31 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GFQmFKvkjr9N for ; Thu, 4 Oct 2018 07:48:31 -0400 (EDT) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 014072A841 for ; Thu, 4 Oct 2018 07:48:30 -0400 (EDT) Date: Thu, 4 Oct 2018 14:48:27 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH] sql: refactor SQL cursor to remove write ones Message-ID: <20181004114827.d2dphzykfpcpdwyg@tarantool.org> References: <4aec5b52bc28f47ad2af0ac6f4e64be0712f378b.1538389679.git.kyukhin@tarantool.org> <7816B3E4-321F-446F-B19A-ED3E241D5D86@tarantool.org> <20181003095759.2cdk54garm4na3f3@tarantool.org> <4A0EC663-775A-4D4B-8458-A27ADB72B71C@tarantool.org> <20181004071637.jncwrcmp62r7zucn@tarantool.org> <036AB308-449D-4789-9F78-432CB93DF902@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <036AB308-449D-4789-9F78-432CB93DF902@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" Cc: tarantool-patches@freelists.org 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