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 44B3530390 for ; Fri, 31 May 2019 18:36:58 -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 Odg4bxk35BkF for ; Fri, 31 May 2019 18:36:58 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 949EE3011B for ; Fri, 31 May 2019 18:36:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class References: <5672cd4b-0c46-b668-efa6-a6a465e601fa@tarantool.org> <20190531194524.GH6141@atlas> From: Vladislav Shpilevoy Message-ID: <1bc3cdb8-2f1d-ebb1-282a-27876d8909dc@tarantool.org> Date: Sat, 1 Jun 2019 00:36:53 +0200 MIME-Version: 1.0 In-Reply-To: <20190531194524.GH6141@atlas> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Konstantin Osipov , tarantool-patches@freelists.org, Kirill Shcherbatov On 31/05/2019 22:45, Konstantin Osipov wrote: > * Kirill Shcherbatov [19/05/31 20:34]: >> Hi! Thank you for review. > > We have a yet enother -er class. It seems not to be a coincidence > that all these -er classes are coming from the same contributor. > > Kirill, please rethink the design, I kind of hit the limit of > active classes. > I would agree with Kirill here. 1) Number of classes does not tell anything how good code is. If a task can be solved with a new class better and simpler than with modification of an existing one, then it is ok. Classes genocide does not solve any problem. 2) I do not understand this dig on Kirill. He works on SQL, introduces brand new features, which have never been presented in Tarantool before, in any kind. You could with the same manner dig on yourself, when you added MemtxSpace, MemtxEngine, MemtxTreeIndex, MemtxHashIndex etc Memtx* + Vinyl* classes. Or when we were adding new classes for vinyl internals like vy_run_iterator, vy_write_iterator, vy_cache_iterator, vy_any_other_shit_iterator etc. Or when *you* asked Nikita to add these numerous useless classes like "create_table_def", "create_constraint_def", "alter_def" (see parse_def.h for the others), which just store some values earlier passed as function parameters. A class is ok, if it solves one atomic simple task, IMO. In this concrete patch tuple_fetcher solves the task of not copying tuple data into Vdbe cells, but still fetching fields. 3) The last point by order, but not by importance - the whole 'fetch' theme was your idea. And I good one, to be honest. You saw a proposed solution weeks ago. I explained it in details. Even if you do not like it now for an unknown reason, it is still good and should be implemented. It makes checks *copy-free*. We do not copy any single byte from a tuple. We only keep offsets to its parts. And even more - multiple checks share the same fetcher now, and if a first check filled the offset table, the next checks will reuse it. Sorry, if you feel that this email insults you, that was not on purpose. Just my thoughts about your actions.