From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 4 Apr 2019 11:38:24 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module Message-ID: <20190404083823.adyv5zxrlgobfro2@esperanza> References: <20190328020146.lluz4mg5tacpghwv@tkn_work_nb> <35ed4661-9789-7cf1-6627-2ced2a821939@tarantool.org> <6d915212-e80f-4a6d-d884-b838bf25f8a7@tarantool.org> <20190328112158.kpxsk6b55noicbes@tkn_work_nb> <20190403111003.x7vq7olda55tthgi@esperanza> <20190404050733.2xuobbezfzbs47l4@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190404050733.2xuobbezfzbs47l4@tkn_work_nb> To: Alexander Turenko Cc: Kirill Shcherbatov , tarantool-patches@freelists.org List-ID: On Thu, Apr 04, 2019 at 08:07:34AM +0300, Alexander Turenko wrote: > > Also, key_def.compare() sounds like it compares key definitions, not > > tuples. May be, we should move these functions to box.tuple module? > > I'm tentative about that. The key_def Lua module is planned to be the > interface to comparators and here we're using comparators. I don't like > spreading of such function across several modules. Maybe 'key_def' name > is not good and we need to use something dedicated from the word > 'comparator'? It's not just a comparator. It's also used for extracting keys. Regarding moving this code to box.tuple - I don't insist. Let's just solicit approval from Kostja on this one. > > > > > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better > > introduce 'equal', 'greater', 'less', etc helpers returning bool? > > A function for table.sort(table, func) returns boolean, so it make > sense. I'm a bit afraid that we'll need to make two calls: say, :less() > and :equal() to determine an order of tuples strictly. But I cannot > provide a case where it can be necessary. Are you know one? No, I don't write much code in Lua. However, if we decided to switch to bool return parameter, I'd implement all possible combinations, i.e. eq, ge, le, gt, lt. > > > > > I'm not strongly against the proposed API, but I think we should agree > > on it with other members of the team, potential users, and Kostja. > > I propose to discuss the questions you arose between us and then send > RFC email for the API (something very like docbot comment we already > have). Agree, an RFC letter with this API and possible alternatives would be good. > > > > +struct key_def * > > > +check_key_def(struct lua_State *L, int idx) > > > > Please prefix the name with lbox_ or... I dunno - the naming looks > > inconsistent: luaT_key_def_set_part, lbox_push_key_part, check_key_def. > > Is there some kind of pattern? > > I understood the convention so: luaL/luaT is somewhat that operates on a > Lua stack / state, but cannot be called from Lua directly (because > either receive or return C values). So luaT_key_def_set_part() looks > right, but lbox_push_key_part(), lbox_key_def_check_tuple() and > check_key_def() seems to need be prefixed with luaT. > > I'll also update check_ibuf(), check_merger_source() and > check_merger_context() in the merger patchset (they are statis however). Okay. I just want to see some pattern, at least in the scope of the same source file. > > > > +/** > > > + * Take existent tuple from LUA stack or build a new tuple with > > > + * default format from table, check for compatibility with a > > > + * given key_def. Take tuple reference pointer on success. > > > + */ > > > +static struct tuple * > > > +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx) > > > +{ > > > + struct tuple *tuple = luaT_istuple(L, idx); > > > + if (tuple == NULL) > > > + tuple = luaT_tuple_new(L, idx, box_tuple_format_default()); > > > + if (tuple == NULL) > > > + return NULL; > > > + /* Check that tuple match with the key definition. */ > > > + uint32_t min_field_count = > > > + tuple_format_min_field_count(&key_def, 1, NULL, 0); > > > + uint32_t field_count = tuple_field_count(tuple); > > > + if (field_count < min_field_count) { > > > + diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1); > > > + return NULL; > > > + } > > > + for (uint32_t idx = 0; idx < key_def->part_count; idx++) { > > > + struct key_part *part = &key_def->parts[idx]; > > > + const char *field = tuple_field_by_part(tuple, part); > > > + if (field == NULL) { > > > + assert(key_def->has_optional_parts); > > > + continue; > > > + } > > > + if (key_part_validate(part->type, field, idx, > > > + key_part_is_nullable(part)) != 0) > > > + return NULL; > > > + } > > > + tuple_ref(tuple); > > > + return tuple; > > > +} > > > > The code checking a tuple against key_def should live somewhere in > > src/box - chances are high that we miss lua/key_def.c when we extend > > key_def struct again. > > Can you suggest where it is better to place this code: src/box/key_def.c > or src/box/tuple.c? Actually, I was thinking about reusing tuple_validate() for this to avoid code duplication. However, if we decide to introduce a separate function, I guess it should live either in key_def.c or tuple_format.c. > > > > +LUA_API int > > > +luaopen_key_def(struct lua_State *L) > > > +{ > > > + luaL_cdef(L, "struct key_def;"); > > > + key_def_type_id = luaL_ctypeid(L, "struct key_def&"); > > > + > > > + /* Export C functions to Lua. */ > > > + static const struct luaL_Reg meta[] = { > > > + {"new", lbox_key_def_new}, > > > + {NULL, NULL} > > > + }; > > > + luaL_register_module(L, "key_def", meta); > > > + > > > + lua_newtable(L); /* key_def.internal */ > > > + lua_pushcfunction(L, lbox_key_def_extract_key); > > > + lua_setfield(L, -2, "extract_key"); > > > + lua_pushcfunction(L, lbox_key_def_compare); > > > + lua_setfield(L, -2, "compare"); > > > + lua_pushcfunction(L, lbox_key_def_compare_with_key); > > > + lua_setfield(L, -2, "compare_with_key"); > > > + lua_pushcfunction(L, lbox_key_def_merge); > > > + lua_setfield(L, -2, "merge"); > > > + lua_pushcfunction(L, lbox_key_def_to_table); > > > + lua_setfield(L, -2, "totable"); > > > + lua_setfield(L, -2, "internal"); > > > > Why 'internal'? We use them as is as key_def methods. > > E.g. box.tuple.* methods aren't internal. > > To distinguish between module and instance methods and don't confuse a But 'compare', 'merge', etc can be used both as module methods and as instance method, i.e. neither of the ways of using say compare is wrong: k = key_def.new(...) k:compare(t1, t2) key_def.compare(k, t1, t2) > user with, say, tab completion in a console. fio.c does the same. I like to see all public methods suggested to me when I type key_def.. Don't know about fio, but box.tuple does the same: t:update(...) box.tuple.update(t, ...) are equally correct AFAIU. > However using, say, :map(box.tuple.totable) is > convenient, so maybe it worth to name this table, say, > 'key_def.instance'?