From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module 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> From: Kirill Shcherbatov Message-ID: <1a4f5fe8-c2d3-47eb-f596-a2104de182fb@tarantool.org> Date: Thu, 4 Apr 2019 11:04:10 +0300 MIME-Version: 1.0 In-Reply-To: <20190404050733.2xuobbezfzbs47l4@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Alexander Turenko , Vladimir Davydov List-ID: >> 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? Due to the fact that this code needs table_field_by_part and tuple_format_min_field_count, it cannot be placed in key_def.c Placing it in tuple.h is not quite correct either; that's why we've got it inlined. >> Probably, we should reuse tuple_validate() for checking a tuple against >> a key_def so as not to implement the same code again. Unfortunately tuple_validate() is designed for format validation while we don't have format here and I don't like create it for validation event in case of error. As for assertion you've found, that is my fault; required fix doesn't increase code complexity (not sure about error message): @@ -243,8 +242,11 @@ lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int 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_is_nullable(part)) + continue; + diag_set(IllegalParams, "tuple doesn't match key " + "definition (part %d)", idx); + return NULL; } if (key_part_validate(part->type, field, idx, key_part_is_nullable(part)) != 0) >> Mixing ER_WRONG_INDEX_OPTIONS and IllegalParams looks ugly. Please fix. @@ -143,8 +143,7 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts, size_t path_len; const char *path = lua_tolstring(L, -1, &path_len); if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) { - diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, - part_idx + TUPLE_INDEX_BASE, "invalid path"); + diag_set(IllegalParams, "invalid path: \"%s\"", path); return -1; } I quote Alexander's message because it contains answers and suggestions for other questions. On 04.04.2019 8:07, Alexander Turenko wrote: >>> 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'?