Re: [Tarantool-patches] [PATCH v2] key_def: support composite types extraction

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5507 bytes --]


Hi!

Sent v3 considering your comments for Alexander L. to review it. 
>Четверг, 1 октября 2020, 14:47 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>LGTM.
>
>I see a bit different way to do the same, but I don't know whether it is
>better. So I would say that both ways are okay for me, but it would be
>good if someone other would look into this discussion too. I hope
>Alexander L. will do.
>
>See also the comment regarding the dubious test case.
>
>WBR, Alexander Turenko.
>
>> + /**
>> + * First part type not supported for comparison.
>> + * Valid if key_def does not support comparison
>> + * (key_def->tuple_compare* == NULL), undefined othewise.
>
>I would set it to field_type_MAX in the case, but maybe it is just my
>fear of undefined values.
>
>> + */
>> + enum field_type unsupported_type;
>
>I see the alternative (but I don't know whether it is better). We can
>wrap the loop into a function and use it directly on the error path
>(when key_def->tuple_compare or key_def->tuple_compare_with_key is NULL)
>to obtain an error message. A performance of the error path is not
>important I guess. This way we'll not need the extra field in <struct
>key_def>.
Right, i think it is a good idea. Didn’t think that we can just do it on the error path.
I will implement it this way, but without extra diag_set().
>
>Unlikely we'll create a lot of key_defs, so the question is purely how
>the code would look better. A field just to pass it to an error message
>is a bit artificial thing, IMHO.
>
>Let the check function (key_def_has_comparator() or so) to set the
>diagnostics and so the checking code and the construction of an error
>message will be in one place. The usage would be like so:
>
>In key_def_set_compare_func():
>
> | if (! key_def_has_comparator(def)) {
> | def->tuple_compare = NULL;
> | def->tuple_compare_with_key = NULL;
> | }
>
>In lbox_key_def_compare():
>
> | if (key_def->tuple_compare == NULL) {
> | /* Just to set an error to the diagnostics area. */
> | int rc = key_def_has_comparator(key_def);
> | (void) rc;
> | assert(rc != 0);
> | return luaT_error(L);
> | }
>
>But I'll repeat: I don't know whether it worth to do. Feel free to
>ignore.
>
>> + if (key_def->tuple_compare == NULL) {
>> + diag_set(IllegalParams, "Unsupported field type: %s",
>> + field_type_strs[key_def->unsupported_type]);
>> + 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 +343,12 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>> "compare_with_key(tuple, key)");
>> }
>>
>> + if (key_def->tuple_compare_with_key == NULL) {
>> + diag_set(IllegalParams, "Unsupported field type: %s",
>> + field_type_strs[key_def->unsupported_type]);
>> + return luaT_error(L);
>> + }
>> +
>> struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);
>> if (tuple == NULL)
>> return luaT_error(L);
>
>(Left here for context.)
>
>> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
>> index d059c709f..762bc8019 100644
>> --- a/src/box/tuple_compare.cc
>> +++ b/src/box/tuple_compare.cc
>> @@ -2081,5 +2081,16 @@ key_def_set_compare_func(struct key_def *def)
>> key_def_set_compare_func_json<false, false>(def);
>> }
>> }
>> + for (uint32_t i = 0; i < def->part_count; ++i) {
>> + if (def->parts[i].type == FIELD_TYPE_ANY ||
>> + def->parts[i].type == FIELD_TYPE_ARRAY ||
>> + def->parts[i].type == FIELD_TYPE_MAP) {
>> + /* Tuple comparators don't support these types. */
>> + def->tuple_compare = NULL;
>> + def->tuple_compare_with_key = NULL;
>> + def->unsupported_type = def->parts[i].type;
>> + break;
>> + }
>> + }
>> key_def_set_hint_func(def);
>> }
>
>(Left here for context.)
>
>> @@ -488,6 +525,35 @@ test:test('merge()', function(test)
>> 'case 3: verify with :totable()')
>> test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),
>> {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')
>> +
>> + local parts_unsigned = {
>> + {type = 'unsigned', fieldno = 1, is_nullable = false},
>> + }
>> + local key_def_unsigned = key_def_lib.new(parts_unsigned)
>> + local key_def_string = key_def_lib.new({
>> + {type = 'string', fieldno = 1},
>> + })
>> + local key_def_array = key_def_lib.new({
>> + {type = 'array', fieldno = 1},
>> + {type = 'unsigned', fieldno = 2},
>> + })
>> + local key_def_map = key_def_lib.new({
>> + {type = 'map', fieldno = 3, is_nullable = true},
>> + {type = 'scalar', fieldno = 2},
>> + })
>> +
>> + local key_def_unsigned_string = key_def_unsigned:merge(key_def_string)
>> + test:is_deeply(key_def_unsigned_string:totable(), parts_unsigned,
>> + 'in case of conflict we just get the field from the first key_def')
>
>If you add a test case and not sure whether corrsponding behaviour is
>right, there are two options. Either clarify that you just hold current
>behaviour and presence of this test case does not mean that the
>behaviour should remain the same in future (but what is purpose of the
>case so?). Or don't add it.
Right! I will clarify the fact this behavior is not intended to be saved.
Though i think it is useful to make it transparent how it works now.
>
>Just to avoid any possible confusion like 'we test the behaviour, so it
>seems there is some commitment that we'll keep it'.
>
>I shared my doubts about this behaviour in [1].
>
>[1]:  https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019807.html 
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 7069 bytes --]