Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko <alexander.turenko@tarantool.org>]

> > 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'?