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

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

[Ilya Kosarev]

key_def didn't support key definitions with array, map, varbinary & any
fields. Thus they couldn't be extracted with
key_def_object:extract_key(). Since the restriction existed due to
impossibility of such types comparison, this patch removes the
restriction for the fields extraction and only leaves it for
comparison.

Closes #4538
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4538-key_def-extract-arrays
Issue: https://github.com/tarantool/tarantool/issues/4538

@ChangeLog:
 * Composite types extraction is now supported in key_def (gh-4538).

Changes in v2:
 - type check is now only performed on key_def creation
 - more tests added

 src/box/key_def.h             | 10 ++++-
 src/box/lua/key_def.c         | 26 ++++++------
 src/box/tuple_compare.cc      | 11 +++++
 test/box-tap/key_def.test.lua | 76 ++++++++++++++++++++++++++++++++---
 4 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/src/box/key_def.h b/src/box/key_def.h
index f4d9e76f2..afea32cc8 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -255,8 +255,14 @@ struct key_def {
 	 * path index key_def::multikey_path.
 	 * Valid when key_def->is_multikey is true,
 	 * undefined otherwise.
-	*/
+	 */
 	uint32_t multikey_fieldno;
+	/**
+	 * First part type not supported for comparison.
+	 * Valid if key_def does not support comparison
+	 * (key_def->tuple_compare* == NULL), undefined othewise.
+	 */
+	enum field_type unsupported_type;
 	/** The size of the 'parts' array. */
 	uint32_t part_count;
 	/** Description of parts of a multipart index. */
@@ -650,6 +656,7 @@ tuple_compare(struct tuple *tuple_a, hint_t tuple_a_hint,
 	      struct tuple *tuple_b, hint_t tuple_b_hint,
 	      struct key_def *key_def)
 {
+	assert(key_def->tuple_compare != NULL);
 	return key_def->tuple_compare(tuple_a, tuple_a_hint,
 				      tuple_b, tuple_b_hint, key_def);
 }
@@ -672,6 +679,7 @@ tuple_compare_with_key(struct tuple *tuple, hint_t tuple_hint,
 		       const char *key, uint32_t part_count,
 		       hint_t key_hint, struct key_def *key_def)
 {
+	assert(key_def->tuple_compare_with_key != NULL);
 	return key_def->tuple_compare_with_key(tuple, tuple_hint, key,
 					       part_count, key_hint, key_def);
 }
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 1a99fab63..fb0b91665 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -127,21 +127,9 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part,
 	const char *type_name = lua_tolstring(L, -1, &type_len);
 	lua_pop(L, 1);
 	part->type = field_type_by_name(type_name, type_len);
-	switch (part->type) {
-	case FIELD_TYPE_ANY:
-	case FIELD_TYPE_VARBINARY:
-	case FIELD_TYPE_ARRAY:
-	case FIELD_TYPE_MAP:
-		/* Tuple comparators don't support these types. */
-		diag_set(IllegalParams, "Unsupported field type: %s",
-			 type_name);
-		return -1;
-	case field_type_MAX:
+	if (part->type == field_type_MAX) {
 		diag_set(IllegalParams, "Unknown field type: %s", type_name);
 		return -1;
-	default:
-		/* Pass though. */
-		break;
 	}
 
 	/* Set part->is_nullable and part->nullable_action. */
@@ -316,6 +304,12 @@ lbox_key_def_compare(struct lua_State *L)
 				     "compare(tuple_a, tuple_b)");
 	}
 
+	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);
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);
 }
diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
index 3a4aad687..43d5b0b37 100755
--- a/test/box-tap/key_def.test.lua
+++ b/test/box-tap/key_def.test.lua
@@ -215,7 +215,7 @@ end
 
 -- Case: extract_key().
 test:test('extract_key()', function(test)
-    test:plan(13)
+    test:plan(14)
 
     local key_def_a = key_def_lib.new({
         {type = 'unsigned', fieldno = 1},
@@ -245,6 +245,18 @@ test:test('extract_key()', function(test)
     }):extract_key({{a = {b = 'foo'}}}):totable()
     test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
 
+    -- Composite types.
+    local tuple = {{{key = 'aa', data = 'aaaa'},
+                    {key = 'ab', data = 'aabb'},
+                    {key = 'bb', data = 'bbbb'}}, box.NULL,
+                    {a = 5, b = 6}}
+    local res = key_def_lib.new({
+        {type = 'array', fieldno = 1},
+        {type = 'map', fieldno = 2, is_nullable = true},
+        {type = 'any', fieldno = 3},
+    }):extract_key(tuple):totable()
+    test:is_deeply(res, tuple, 'Composite types')
+
     -- A key def has a **nullable** part with a field that is over
     -- a tuple size.
     --
@@ -335,7 +347,7 @@ end)
 
 -- Case: compare().
 test:test('compare()', function(test)
-    test:plan(8)
+    test:plan(9)
 
     local key_def_a = key_def_lib.new({
         {type = 'unsigned', fieldno = 1},
@@ -365,11 +377,19 @@ test:test('compare()', function(test)
             'case 3: less (table argument)')
     test:is(key_def_b:compare(tuple_a:totable(), tuple_c:totable()), 0,
             'case 4: equal (table argument)')
+
+    local cmp_err = 'Unsupported field type: array'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'array', fieldno = 2, is_nullable = true},
+    })
+    local ok, err = pcall(key_def.compare, key_def, {'aa', {}}, {'bb', box.NULL})
+    test:is_deeply({ok, tostring(err)}, {false, cmp_err}, 'no composite comparison')
 end)
 
 -- Case: compare_with_key().
 test:test('compare_with_key()', function(test)
-    test:plan(2)
+    test:plan(3)
 
     local key_def_b = key_def_lib.new({
         {type = 'number', fieldno = 2},
@@ -382,11 +402,19 @@ test:test('compare_with_key()', function(test)
 
     local key = box.tuple.new({1, 22})
     test:is(key_def_b:compare_with_key(tuple_a, key), 0, 'tuple')
+
+    local cmp_err = 'Unsupported field type: map'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'map', fieldno = 2},
+    })
+    local ok, err = pcall(key_def.compare_with_key, key_def, {'aa', {}}, {'bb', box.NULL})
+    test:is_deeply({ok, tostring(err)}, {false, cmp_err}, 'no composite comparison')
 end)
 
 -- Case: totable().
 test:test('totable()', function(test)
-    test:plan(2)
+    test:plan(3)
 
     local parts_a = {
         {type = 'unsigned', fieldno = 1}
@@ -395,14 +423,23 @@ test:test('totable()', function(test)
         {type = 'number', fieldno = 2},
         {type = 'number', fieldno = 3},
     }
+    local parts_composite = {
+        {type = 'number', fieldno = 2},
+        {type = 'array', fieldno = 3},
+        {type = 'map', fieldno = 4, is_nullable = true},
+    }
     local key_def_a = key_def_lib.new(parts_a)
     local key_def_b = key_def_lib.new(parts_b)
+    local key_def_composite = key_def_lib.new(parts_composite)
 
     local exp = set_key_part_defaults(parts_a)
     test:is_deeply(key_def_a:totable(), exp, 'case 1')
 
     local exp = set_key_part_defaults(parts_b)
     test:is_deeply(key_def_b:totable(), exp, 'case 2')
+
+    local exp = set_key_part_defaults(parts_composite)
+    test:is_deeply(key_def_composite:totable(), exp, 'composite case')
 end)
 
 -- Case: __serialize().
@@ -447,7 +484,7 @@ end)
 
 -- Case: merge().
 test:test('merge()', function(test)
-    test:plan(6)
+    test:plan(8)
 
     local key_def_a = key_def_lib.new({
         {type = 'unsigned', fieldno = 1},
@@ -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')
+
+    local key_def_array_map = key_def_array:merge(key_def_map)
+    local exp_parts = {
+        {type = 'array', fieldno = 1, is_nullable = false},
+        {type = 'unsigned', fieldno = 2, is_nullable = false},
+        {type = 'map', fieldno = 3, is_nullable = true},
+    }
+    test:is_deeply(key_def_array_map:totable(), exp_parts,
+        'composite case')
 end)
 
 os.exit(test:check() and 0 or 1)
-- 
2.17.1

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

[Alexander Turenko]

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>.

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.

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

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

[Ilya Kosarev]

[-- 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 --]