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 4F13330668 for ; Sat, 1 Jun 2019 05:16:32 -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 ACHpAbZ95GMa for ; Sat, 1 Jun 2019 05:16:31 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 76BC230666 for ; Sat, 1 Jun 2019 05:16:31 -0400 (EDT) Date: Sat, 1 Jun 2019 08:45:46 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class Message-ID: <20190601054546.GB19405@atlas> References: <5672cd4b-0c46-b668-efa6-a6a465e601fa@tarantool.org> <20190531194524.GH6141@atlas> <1bc3cdb8-2f1d-ebb1-282a-27876d8909dc@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1bc3cdb8-2f1d-ebb1-282a-27876d8909dc@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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org, Kirill Shcherbatov * Vladislav Shpilevoy [19/06/01 06:03]: I had very little time to react to the patch, so this was a bit of a rushed response. I will take time to rethink the class layout with Kirill to avoid yet-another-er. I have already suggested in the channel that instead of a fetcher, one should use a proxy-like class, like vdbe_field_proxy, or vdbe_tuple_proxy. Perhaps many of the fetcher methods would be the same, but the object-oriented approach makes you reason about responsibility layout differently. E.g. vdbe_tuple_proxy could have just the same api as the tuple itself, and contain an internal cache for the already fetched fields. But generally this doesn't diminish the fact that this design choice was made and passed the review, even though abuse of "active" classes is a well known anti-pattern. I encourage you to read up on this, so that I don't have to "lecture" anyone. > > 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. -- Konstantin Osipov, Moscow, Russia