From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 4 Apr 2019 08:07:34 +0300 From: Alexander Turenko Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module Message-ID: <20190404050733.2xuobbezfzbs47l4@tkn_work_nb> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190403111003.x7vq7olda55tthgi@esperanza> To: Vladimir Davydov Cc: Kirill Shcherbatov , tarantool-patches@freelists.org List-ID: > > A key_def instance has the following methods: > > > > * :extract_key(tuple) -> key (as tuple) > > * :compare(tuple_a, tuple_b) -> number > > * :compare_with_key(tuple, key) -> number > > What number do these functions return? > > > * :merge(another_key_def) -> new key_def instance > > What does 'merge' do? > > > * :totable() -> table > > Does this function return key_def parts? In what format? > Please elaborate the comments. Note: I think it worth to leave this list of brief descriptions in this format and describe meaning of arguments and return values for each function below. > > 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'? > > 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? > > 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). > > +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). > > +/** > > + * 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? > > +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 user with, say, tab completion in a console. fio.c does the same. However using, say, :map(box.tuple.totable) is convenient, so maybe it worth to name this table, say, 'key_def.instance'?