[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Konstantin Osipov <kostja@tarantool.org>]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/03/27 09:35]:

Could you please send the final patch for review?

> I would prefer if you will answer to review comments explicitly.
> 
> Re using lua_tolstring(): I understood that lua_tolstring() only
> guarantees that a string is valid until it will be popped from a stack
> (despite that fact that we know it has another copy on the stack) and so
> a region usage looks good.
> 
> Just for the record: We discussed voicely that make this patch depending
> on the luaT_tuple_new() implementation from the merger patchset is not
> worth to do (just saves two lines of code here).
> 
> Changes I made (I mostly add them into a separate 'FIXUP' commit to
> allow you to look into them if you want; pushed to your branch back):
> 
> * Squashed the commits, updated the commit message.
> * Renamed :to_table() to totable().
> * Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0'
>   assignment (lua_tolstring() allows it).
> * Use 'struct lua_State' instead of just 'lua_State' to better match
>   with our code style.
> * Rewrote a test: split it by cases, do some renaming, fixed mine
>   parts = {...} to parts = {{...}}, added more cases.
> * Found an assertion fault with a new test case:
> 
> tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed.
> Aborted
> 
> See the first commented test case. I think that we should allow a
> fieldno in key_def that is above then a tuple length when the field is
> nullable and raise an error otherwise (when it is not nullable). (I
> guess that we can do it at least partially by setting
> has_optional_parts).
> 
> However when I tried with non-nullable part with {fieldno = 2}
> :extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its
> memory. But it asserts like the first one if I added {fieldno = 3}.
> 
> All that cases are commented now. Can you please elaborate how to
> archieve the behaviour I proposed above and whether it is good way to
> go, Kirill?
> 
> Also I have an idea: maybe add ability to use tables as tuples? We have
> to receive tuples in this way from net.box.
> 
> WBR, Alexander Turenko.
> 
> On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote:
> > Hi! Thank you for review.
> > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
> > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov