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

[Vladimir Davydov]

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.<TAB><TAB>. Don't know about fio, but box.tuple does the same:

	t:update(...)
	box.tuple.update(t, ...)

are equally correct AFAIU.

> However using, say, <luafun iterator>:map(box.tuple.totable) is
> convenient, so maybe it worth to name this table, say,
> 'key_def.instance'?