[Tarantool-patches] [PATCH] key_def: support composite types extraction
Alexander Turenko
alexander.turenko at tarantool.org
Fri Sep 25 21:34:50 MSK 2020
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.
> +{
> + 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.
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.
Everything looks good with lbox_key_def_to_table(), but I would add a
test anyway.
More information about the Tarantool-patches
mailing list