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 D54902FAAC for ; Wed, 5 Jun 2019 08:07:50 -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 zxPfGNchPM7B for ; Wed, 5 Jun 2019 08:07:50 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 806AA2FA3B for ; Wed, 5 Jun 2019 08:07:50 -0400 (EDT) Date: Wed, 5 Jun 2019 09:47:06 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class Message-ID: <20190605064706.GA16746@atlas> References: <5672cd4b-0c46-b668-efa6-a6a465e601fa@tarantool.org> <20190531194524.GH6141@atlas> <20c02296-1a80-a384-cafa-c061f51ad31d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20c02296-1a80-a384-cafa-c061f51ad31d@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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy * Kirill Shcherbatov [19/06/03 14:34]: > Hi! I've renamed tuple_fetcher to vdbe_field_ref as you've proposed. > Here and everywhere. The corresponding diff below: > A rename is only the first step. It's also about managing the responsibility. As far as I can see, the new class is a proxy for an entire tuple, not a single field. This doesn't seem convenient going forward, but since there is a shared state (the slots array), let it be. But then the name is wrong, it's vdbe_tuple_ref, not vdbe_field_ref. Why did you blindly rename without objecting to a another stupid name? The next thing is managing the responsibility for the tuple itself. If we begin to think that this is a tuple ref from vdbe, then this ref should own the tuple, and vdbe should always access the tuple using this ref. So vdbe_field_ref_create should ref the tuple, vdbe_field_ref_destroy() should free it. Again, please object if it doesn't make sense - or it may require some refactoring (likely). Finally, the choice of the representation for the slots array and the implementation itself. When initializing the fetcher class, you do not consult with vdbe what tuple fields are used. Yet you do know it from the parsing - and only select fields (one or two) are used in most cases. For example: CHECK age >=0 and age <= 170 -- sane age So you don't need to allocate or store a lot of slots in most cases. Moreover, perhaps it's wise to only use an optimization of there are few slots in use, and cache Mem objects, not offsets to fields, and if there are too many slots in use, do nothing? Please keep in mind that the same object could be used for WHERE, HAVING or other clauses in the future. -- Konstantin Osipov, Moscow, Russia