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

[Tarantool-patches] [PATCH] 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).

 src/box/lua/key_def.c         | 36 ++++++++++++++++++++++-------------
 test/box-tap/key_def.test.lua | 34 ++++++++++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 1a99fab63dc..76ee6c8867d 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. */
@@ -252,6 +240,22 @@ luaT_check_key_def(struct lua_State *L, int idx)
 	return *key_def_ptr;
 }
 
+static bool
+key_def_comparable(struct key_def *key_def)
+{
+	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;
+}
+
 /**
  * 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
--- 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,6 +402,14 @@ 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().
-- 
2.17.1

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

[Alexander Turenko]

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.

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

[Ilya Kosarev]

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


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
 

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

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

[Alexander Turenko]

> >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'm not sure it is correct. But at least you proved that it is not in
the scope of your current task :)

An attempt to create conflicting indices gives an error, so I guess this
case is not handled, because it was not possible until we exposed
key_def directly to Lua.

But another case makes me doubt even more:

 | tarantool> key_def = require('key_def')
 | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'scalar'}})
 | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'integer'}})
 | tarantool> kd1:merge(kd2)
 | ---
 | - - type: scalar
 |     is_nullable: false
 |     fieldno: 1
 | ...
 |
 | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'integer'}})
 | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'scalar'}})
 | tarantool> kd1:merge(kd2)
 | ---
 | - - type: integer
 |     is_nullable: false
 |     fieldno: 1
 | ...

It would be natural to get most restricted type, when one type is subset
of another. I assume the following invariant: for any given kd1 and kd2
kd1:merge(kd2) should accept only those tuples that both kd1 and kd2
accept (kd accepts a tuple when it is valid against kd).

OTOH, the main use case for :merge() function is to compare tuples in
the same way as a non-unique secondary index do (when a key parts of a
primary index are implicitly merged into the secondary index ones). In
this sense we can choose either subtype or supertype: it will not affect
an order of tuples. So choosing of the first (secondary index's) key
part looks okay.

I don't know whether we should change anything or not, so just shared my
findings and thoughts.

If I'll somehow end with an idea that we should do something here, I'll
file an issue.

WBR, Alexander Turenko.

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

[Ilya Kosarev]

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


Right, I agree with the thoughts on the compatibility.
I think this might be a thing to ask solutions? 
>Четверг, 1 октября 2020, 14:38 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>> >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'm not sure it is correct. But at least you proved that it is not in
>the scope of your current task :)
>
>An attempt to create conflicting indices gives an error, so I guess this
>case is not handled, because it was not possible until we exposed
>key_def directly to Lua.
>
>But another case makes me doubt even more:
>
> | tarantool> key_def = require('key_def')
> | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'scalar'}})
> | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'integer'}})
> | tarantool> kd1:merge(kd2)
> | ---
> | - - type: scalar
> | is_nullable: false
> | fieldno: 1
> | ...
> |
> | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'integer'}})
> | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'scalar'}})
> | tarantool> kd1:merge(kd2)
> | ---
> | - - type: integer
> | is_nullable: false
> | fieldno: 1
> | ...
>
>It would be natural to get most restricted type, when one type is subset
>of another. I assume the following invariant: for any given kd1 and kd2
>kd1:merge(kd2) should accept only those tuples that both kd1 and kd2
>accept (kd accepts a tuple when it is valid against kd).
>
>OTOH, the main use case for :merge() function is to compare tuples in
>the same way as a non-unique secondary index do (when a key parts of a
>primary index are implicitly merged into the secondary index ones). In
>this sense we can choose either subtype or supertype: it will not affect
>an order of tuples. So choosing of the first (secondary index's) key
>part looks okay.
>
>I don't know whether we should change anything or not, so just shared my
>findings and thoughts.
>
>If I'll somehow end with an idea that we should do something here, I'll
>file an issue.
>
>WBR, Alexander Turenko. 
 
 
--
Ilya Kosarev
 

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