Hi,

Thanks for your review!
 
Sent v2 of the patch considering your comments. 
>Пятница, 25 сентября 2020, 21:34 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>I have no objections in general, but there are doubts around several
>places. Please, look below.
>
>WBR, Alexander Turenko.
>
>> +static bool
>> +key_def_comparable(struct key_def *key_def)
>
>What make me doubt: key_def is not comparable per se, it may or may not
>be used for comparison of tuples and a tuple with a key.
>'key_def_has_comparator' or 'key_def_can_compare' (however not key_def
>itself perform comparisons, hmm), maybe, don't know.
Well, yes. I meant key_def as a module affiliation here, not like an object.
Good thing is that we don’t really need separate function with the new approach!
>
>> +{
>> + for (uint32_t i = 0; i < key_def->part_count; ++i) {
>> + if (key_def->parts[i].type == FIELD_TYPE_ANY ||
>> + key_def->parts[i].type == FIELD_TYPE_ARRAY ||
>> + key_def->parts[i].type == FIELD_TYPE_MAP) {
>> + /* Tuple comparators don't support these types. */
>> + diag_set(IllegalParams, "Unsupported field type: %s",
>> + field_type_strs[key_def->parts[i].type]);
>> + return false;
>> + }
>> + }
>> + return true;
>> +}
>> +
>
>Ilya gives the idea: perform this check on key_def creation and store a
>flag inside key_def. Check against the flag in lbox_key_def_compare()
>and lbox_key_def_compare_with_key().
>
>This looks as the right way to solve this kind of problems: comparisons
>are more hot functions than key_def creation.
>
>We can sink it down to key_def_set_compare_func() and set NULL to
>key_def->{tuple_compare,tuple_compare_with_key}. Than check it in
>lbox_key_def_compare*() and add asserts to tuple_compare*(). No new
>fields will be required so.
Yes! This seems like the right choice. Though we still need a field here:
somewhere to store the unsupported type to show it to the user any time
he tries to compare. Implemented in v2 of the patch.
>
>This part surely should look someone, who is more near to comparators
>than me.
>
>> /**
>> * Free a key_def from a Lua code.
>> */
>> @@ -316,6 +320,9 @@ lbox_key_def_compare(struct lua_State *L)
>> "compare(tuple_a, tuple_b)");
>> }
>>
>> + if (!key_def_comparable(key_def))
>> + return luaT_error(L);
>> +
>> struct tuple *tuple_a, *tuple_b;
>> if ((tuple_a = luaT_key_def_check_tuple(L, key_def, 2)) == NULL)
>> return luaT_error(L);
>> @@ -349,6 +356,9 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>> "compare_with_key(tuple, key)");
>> }
>>
>> + if (!key_def_comparable(key_def))
>> + return luaT_error(L);
>> +
>> struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);
>> if (tuple == NULL)
>> return luaT_error(L);
>> diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
>> index 3a4aad68721..8fcdf7070bf 100755
>
>How about lbox_key_def_merge() and underlying functions? I'm not sure
>they will work correct. At least I tried this on the branch:
>
> | tarantool> key_def = require('key_def')
> | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'array'}})
> | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'map'}})
> | tarantool> kd1:merge(kd2)
> | ---
> | - - type: array
> | is_nullable: false
> | fieldno: 1
> | ...
>
>It does not look correct.
This turns out to be the correct behavior:
 
tarantool> kd1 = key_def.new({{fieldno = 1, type = 'unsigned'}})
---
...
tarantool> kd2 = key_def.new({{fieldno = 1, type = 'string'}})
---
...
tarantool> kd1:merge(kd2)
---
- - type: unsigned
    is_nullable: false
    fieldno: 1
…
 
I decided these functions don’t really interfere and didn’t manage to test.
Although this is kinda true for now, that wasn’t a correct decision, as far as
behavior might be changed. Adding the tests in the v2 of the patch.
>
>Everything looks good with lbox_key_def_to_table(), but I would add a
>test anyway. 
 
 
--
Ilya Kosarev