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.