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

[Kirill Shcherbatov]

>> 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, <luafun iterator>:map(box.tuple.totable) is
> convenient, so maybe it worth to name this table, say,
> 'key_def.instance'?