[tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
Alexander Turenko
alexander.turenko at tarantool.org
Thu Apr 4 08:07:34 MSK 2019
> > 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, <luafun iterator>:map(box.tuple.totable) is
convenient, so maybe it worth to name this table, say,
'key_def.instance'?
More information about the Tarantool-patches
mailing list