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 DF66E26CD7 for ; Tue, 23 Apr 2019 11:06:33 -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 32L8ncdE0pSA for ; Tue, 23 Apr 2019 11:06:33 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 9B11F26CB7 for ; Tue, 23 Apr 2019 11:06:33 -0400 (EDT) Date: Tue, 23 Apr 2019 18:06:31 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object Message-ID: <20190423150631.GC5668@chai> References: <20190326014624.jzrpzwghnrtfx3mr@tkn_work_nb> <2a06406f-3ea0-b892-5c8d-a0ee54e07d60@tarantool.org> <20190326225408.tuqvvlhqp3mqipt5@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190326225408.tuqvvlhqp3mqipt5@tkn_work_nb> 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: tarantool-patches@freelists.org Cc: Kirill Shcherbatov * Alexander Turenko [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