[tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [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 <kshcherbatov@tarantool.org> [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