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

[Alexander Turenko]

On Thu, Apr 04, 2019 at 11:38:24AM +0300, Vladimir Davydov wrote:
> 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.

It is a comparator except extracting keys. Or keys-related functions
except comparing a tuple against a tuple. Now I don't sure.

> > 
> > > 
> > > 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 mean are there cases when we need to check, say t1 < t2 and if it is
false then check whether t1 == t2 or t1 > t2? In other words, cases when
we need to distinguish all three possible situations. Some algorithms?

I looked throught [1] and it seems it does not contain any reason why
<=> operator was added to C++20 except ones related to comparison
operations autogeneration.

We can just add :cmp() in addition to all other variants, what do you
think?

We can also do the trick: return {eq = <boolean>} from :le() / :ge()
instead of true, but this way looks weird.

[1]: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0515r0.pdf

> > > > +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.

Hm. I didn't thought about that in this way. Okay.

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