Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class
Date: Sat, 1 Jun 2019 08:45:46 +0300	[thread overview]
Message-ID: <20190601054546.GB19405@atlas> (raw)
In-Reply-To: <1bc3cdb8-2f1d-ebb1-282a-27876d8909dc@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

  reply	other threads:[~2019-06-01  9:16 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-23 10:19 [tarantool-patches] [PATCH v5 0/6] box: run checks on insertions in LUA spaces Kirill Shcherbatov
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 1/6] sql: introduce a new method to bind a pointer Kirill Shcherbatov
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 2/6] sql: refactor OP_Column vdbe instruction Kirill Shcherbatov
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 3/6] sql: introduce tuple_fetcher class Kirill Shcherbatov
2019-05-26 12:05   ` [tarantool-patches] " Vladislav Shpilevoy
2019-05-31 13:45     ` Kirill Shcherbatov
2019-05-31 19:45       ` Konstantin Osipov
2019-05-31 19:50         ` Kirill Shcherbatov
2019-05-31 22:36         ` Vladislav Shpilevoy
2019-06-01  5:45           ` Konstantin Osipov [this message]
2019-06-02 18:50         ` Kirill Shcherbatov
2019-06-03 21:15           ` Vladislav Shpilevoy
2019-06-05  6:47           ` Konstantin Osipov
2019-06-05  6:48             ` Konstantin Osipov
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 4/6] schema: add new system space for CHECK constraints Kirill Shcherbatov
2019-05-26 12:06   ` [tarantool-patches] " Vladislav Shpilevoy
2019-05-26 13:31     ` n.pettik
2019-05-26 13:32       ` Vladislav Shpilevoy
2019-05-31 13:45     ` Kirill Shcherbatov
2019-06-03 21:15       ` Vladislav Shpilevoy
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 5/6] box: run check constraint tests on space alter Kirill Shcherbatov
2019-05-26 12:07   ` [tarantool-patches] " Vladislav Shpilevoy
2019-05-31 13:45     ` Kirill Shcherbatov
2019-06-03 21:15       ` Vladislav Shpilevoy
2019-05-23 10:19 ` [tarantool-patches] [PATCH v5 6/6] box: user-friendly interface to manage ck constraints Kirill Shcherbatov
2019-05-26 12:07   ` [tarantool-patches] " Vladislav Shpilevoy
2019-05-31 13:45     ` Kirill Shcherbatov
2019-06-03 21:15 ` [tarantool-patches] Re: [PATCH v5 0/6] box: run checks on insertions in LUA spaces Vladislav Shpilevoy
2019-06-04  7:21   ` Kirill Shcherbatov
2019-06-04 18:59     ` Vladislav Shpilevoy
2019-06-06 11:58 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190601054546.GB19405@atlas \
    --to=kostja@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH v5 3/6] sql: introduce tuple_fetcher class' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox