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

[Konstantin Osipov <kostja@tarantool.org>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [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