[Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index

[Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index

[Ilya Kosarev]

Multikey index did not work properly with nullable root field in
tuple_raw_multikey_count(). Now it is fixed and corresponding
restrictions are dropped. This also means that we can drop implicit
nullability update for array/map fields and make all fields nullable
by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435
(tuple format: don't allow null where array/map is expected), as far as
default non-nullability itself doesn't solve any real problems while
providing confusing behavior (gh-5027).

Follow-up #5027
Closes #5192

@TarantoolBot document
Title: tuple: allow nullable multikey index root
Update the documentation for space_object:create_index() to reflect
that it is now safe for multikey index root to be nullable.
Previously it was not safe to insert, for example, box.NULL as an array
field which elements are the part of multikey index due to error in
counter of multikey index keys in tuple. It was partly fixed using
default non-nullability for tuple fields. In 2.3.3 the restriction on
nullable multikey index root was introduced. Now the counter problem is
fixed and all tuple fields are nullable by default as before 2.2.1.
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
Issue: https://github.com/tarantool/tarantool/issues/5192

@ChangeLog:
 * Dropped restrictions on nullable multikey index root. They were
 introduced due to inaccuracy in multikey index realization. It is
 now fixed. Also all fields are now nullable by default as it was
 before 2.2.1 (gh-5192).

 Changes in v2:
- removed insignificant changes
- fixed tests comments

 Changes in v3:
- fixed some nits
- added docbot request

 src/box/memtx_space.c                         |  8 ---
 src/box/tuple.c                               |  5 +-
 src/box/tuple_format.c                        | 20 ------
 src/box/vinyl.c                               |  8 ---
 test/engine/gh-5027-fields-nullability.result | 63 ++-----------------
 .../gh-5027-fields-nullability.test.lua       | 27 ++------
 .../gh-5192-multikey-root-nullability.result  | 59 +++++++++++++++++
 ...gh-5192-multikey-root-nullability.test.lua | 19 ++++++
 test/engine/json.result                       | 20 +++---
 test/engine/json.test.lua                     |  6 +-
 test/engine/multikey.result                   |  8 +--
 test/engine/multikey.test.lua                 |  4 +-
 12 files changed, 111 insertions(+), 136 deletions(-)
 create mode 100644 test/engine/gh-5192-multikey-root-nullability.result
 create mode 100644 test/engine/gh-5192-multikey-root-nullability.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 8452ab4303..d30ce44b82 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -604,14 +604,6 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 			return -1;
 		}
 	}
-	if (key_def->is_multikey &&
-	    key_def->multikey_fieldno < space->def->field_count &&
-	    space->def->fields[key_def->multikey_fieldno].is_nullable) {
-		diag_set(ClientError, ER_UNSUPPORTED,
-			 "multikey index",
-			 "nullable root field");
-		return -1;
-	}
 	switch (index_def->type) {
 	case HASH:
 		if (! index_def->opts.is_unique) {
diff --git a/src/box/tuple.c b/src/box/tuple.c
index e77753a5ba..f3965476ea 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -555,7 +555,10 @@ tuple_raw_multikey_count(struct tuple_format *format, const char *data,
 					NULL, MULTIKEY_NONE);
 	if (array_raw == NULL)
 		return 0;
-	assert(mp_typeof(*array_raw) == MP_ARRAY);
+	enum mp_type type = mp_typeof(*array_raw);
+	if (type == MP_NIL)
+		return 0;
+	assert(type == MP_ARRAY);
 	return mp_decode_array(&array_raw);
 }
 
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index bae6c67cda..9b817d3cf7 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -242,24 +242,6 @@ tuple_field_ensure_child_compatibility(struct tuple_field *parent,
 	return 0;
 }
 
-/**
- * Tuple fields are nullable by default. However, it is not ok
- * for array/map fields, as far as their implicit nullability
- * might break field accessors expectations, provide confusing
- * error messages and cause incorrect behaviour of
- * tuple_multikey_count(). Thus array/map fields have to be
- * non-nullable by default, which means we have to update default
- * nullability for them.
- */
-static void
-tuple_field_update_nullability(struct tuple_field *field)
-{
-	if ((field->type == FIELD_TYPE_ARRAY ||
-	     field->type == FIELD_TYPE_MAP) &&
-	     tuple_field_is_nullable(field))
-		field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
-}
-
 /**
  * Given a field number and a path, add the corresponding field
  * to the tuple format, allocating intermediate fields if
@@ -335,13 +317,11 @@ tuple_format_add_field(struct tuple_format *format, uint32_t fieldno,
 				parent->offset_slot = *current_slot;
 			}
 		}
-		tuple_field_update_nullability(parent);
 		parent->is_key_part = true;
 		next->is_multikey_part = is_multikey;
 		parent = next;
 		token_count++;
 	}
-	tuple_field_update_nullability(parent);
 	/*
 	 * The path has already been verified by the
 	 * key_def_decode_parts function.
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 32301d7bab..5fa3ea3a54 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -658,14 +658,6 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
 		diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name(space));
 		return -1;
 	}
-	if (key_def->is_multikey &&
-	    key_def->multikey_fieldno < space->def->field_count &&
-	    space->def->fields[key_def->multikey_fieldno].is_nullable) {
-		diag_set(ClientError, ER_UNSUPPORTED,
-		         "multikey index",
-		         "nullable root field");
-		return -1;
-	}
 	/* Check that there are no ANY, ARRAY, MAP parts */
 	for (uint32_t i = 0; i < key_def->part_count; i++) {
 		struct key_part *part = &key_def->parts[i];
diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-fields-nullability.result
index 1121727f6f..1e2709a24e 100644
--- a/test/engine/gh-5027-fields-nullability.result
+++ b/test/engine/gh-5027-fields-nullability.result
@@ -41,79 +41,26 @@ _ = s:create_index('i1', {parts={{1, 'unsigned'}}})
 _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})
  | ---
  | ...
-s:replace{1}
+s:replace{1} -- error
  | ---
  | - error: Tuple field 5 required by space format is missing
  | ...
-s:replace{1, box.NULL}
+s:replace{1, box.NULL} -- error
  | ---
  | - error: Tuple field 5 required by space format is missing
  | ...
-s:replace{1, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL} -- error
  | ---
  | - error: Tuple field 5 required by space format is missing
  | ...
-s:replace{1, box.NULL, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL, box.NULL} -- error
  | ---
  | - error: Tuple field 5 required by space format is missing
  | ...
-s:replace{1, box.NULL, box.NULL, box.NULL, 5}
+s:replace{1, box.NULL, box.NULL, box.NULL, 5} -- ok
  | ---
  | - [1, null, null, null, 5]
  | ...
 s:drop()
  | ---
  | ...
-
-s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
- | ---
- | ...
-_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}})
- | ---
- | ...
-_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
- | ---
- | ...
-s:replace{1, box.NULL}
- | ---
- | - [1, null]
- | ...
-_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
- | ---
- | - error: multikey index does not support nullable root field
- | ...
-s:replace{2, box.NULL}
- | ---
- | - [2, null]
- | ...
-s:drop()
- | ---
- | ...
-
-s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
- | ---
- | ...
-_ = s:format({{name='id'}, {name='data', type='array'}})
- | ---
- | ...
-_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
- | ---
- | ...
-s:replace{1, box.NULL}
- | ---
- | - error: 'Tuple field 2 type does not match one required by operation: expected array'
- | ...
-_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
- | ---
- | ...
-s:replace{2, box.NULL}
- | ---
- | - error: 'Tuple field 2 type does not match one required by operation: expected array'
- | ...
-s:replace{3, {}}
- | ---
- | - [3, []]
- | ...
-s:drop()
- | ---
- | ...
diff --git a/test/engine/gh-5027-fields-nullability.test.lua b/test/engine/gh-5027-fields-nullability.test.lua
index 960103d6c4..8553d488c0 100644
--- a/test/engine/gh-5027-fields-nullability.test.lua
+++ b/test/engine/gh-5027-fields-nullability.test.lua
@@ -12,26 +12,9 @@ s:drop()
 s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
 _ = s:create_index('i1', {parts={{1, 'unsigned'}}})
 _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})
-s:replace{1}
-s:replace{1, box.NULL}
-s:replace{1, box.NULL, box.NULL}
-s:replace{1, box.NULL, box.NULL, box.NULL}
-s:replace{1, box.NULL, box.NULL, box.NULL, 5}
-s:drop()
-
-s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
-_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}})
-_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
-s:replace{1, box.NULL}
-_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
-s:replace{2, box.NULL}
-s:drop()
-
-s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
-_ = s:format({{name='id'}, {name='data', type='array'}})
-_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
-s:replace{1, box.NULL}
-_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
-s:replace{2, box.NULL}
-s:replace{3, {}}
+s:replace{1} -- error
+s:replace{1, box.NULL} -- error
+s:replace{1, box.NULL, box.NULL} -- error
+s:replace{1, box.NULL, box.NULL, box.NULL} -- error
+s:replace{1, box.NULL, box.NULL, box.NULL, 5} -- ok
 s:drop()
diff --git a/test/engine/gh-5192-multikey-root-nullability.result b/test/engine/gh-5192-multikey-root-nullability.result
new file mode 100644
index 0000000000..2a9cd66ed6
--- /dev/null
+++ b/test/engine/gh-5192-multikey-root-nullability.result
@@ -0,0 +1,59 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
+ | ---
+ | ...
+_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}})
+ | ---
+ | ...
+_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
+ | ---
+ | ...
+s:replace{1, box.NULL} -- ok
+ | ---
+ | - [1, null]
+ | ...
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+ | ---
+ | ...
+s:replace{2, box.NULL} -- ok
+ | ---
+ | - [2, null]
+ | ...
+_ = s:delete(2)
+ | ---
+ | ...
+s:drop()
+ | ---
+ | ...
+
+s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
+ | ---
+ | ...
+_ = s:format({{name='id'}, {name='data', type='array'}})
+ | ---
+ | ...
+_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
+ | ---
+ | ...
+s:replace{1, box.NULL} -- error
+ | ---
+ | - error: 'Tuple field 2 type does not match one required by operation: expected array'
+ | ...
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+ | ---
+ | ...
+s:replace{2, box.NULL} -- error
+ | ---
+ | - error: 'Tuple field 2 type does not match one required by operation: expected array'
+ | ...
+s:replace{3, {}} -- ok
+ | ---
+ | - [3, []]
+ | ...
+s:drop()
+ | ---
+ | ...
diff --git a/test/engine/gh-5192-multikey-root-nullability.test.lua b/test/engine/gh-5192-multikey-root-nullability.test.lua
new file mode 100644
index 0000000000..6eb0eaf059
--- /dev/null
+++ b/test/engine/gh-5192-multikey-root-nullability.test.lua
@@ -0,0 +1,19 @@
+test_run = require('test_run').new()
+
+s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
+_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}})
+_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
+s:replace{1, box.NULL} -- ok
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- ok
+_ = s:delete(2)
+s:drop()
+
+s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
+_ = s:format({{name='id'}, {name='data', type='array'}})
+_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
+s:replace{1, box.NULL} -- error
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- error
+s:replace{3, {}} -- ok
+s:drop()
diff --git a/test/engine/json.result b/test/engine/json.result
index 2175266fcc..b8fd9a1b69 100644
--- a/test/engine/json.result
+++ b/test/engine/json.result
@@ -738,12 +738,11 @@ _ = s:create_index('sk', {parts = {{'[2][1].a', 'unsigned'}}})
 ...
 s:insert{1, box.NULL} -- error
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- error: Tuple field [2][1]["a"] required by space format is missing
 ...
 s:insert{2, {box.NULL}} -- error
 ---
-- error: 'Tuple field [2][1] type does not match one required by operation: expected
-    map'
+- error: Tuple field [2][1]["a"] required by space format is missing
 ...
 s:insert{3} -- error
 ---
@@ -764,16 +763,15 @@ s:insert{6, {{a = 1}}} -- ok
 s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
 ---
 ...
-s:insert{7, box.NULL} -- error
+s:insert{7, box.NULL} -- ok
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [7, null]
 ...
-s:insert{8, {box.NULL}} -- error
+s:insert{8, {box.NULL}} -- ok
 ---
-- error: 'Tuple field [2][1] type does not match one required by operation: expected
-    map'
+- [8, [null]]
 ...
--- Skipping nullable fields is okay though.
+-- Skipping nullable fields is also okay.
 s:insert{9} -- ok
 ---
 - [9]
@@ -792,7 +790,9 @@ s:insert{12, {{a = box.NULL}}} -- ok
 ...
 s.index.sk:select()
 ---
-- - [9]
+- - [7, null]
+  - [8, [null]]
+  - [9]
   - [10, []]
   - [11, [{'b': 1}]]
   - [12, [{'a': null}]]
diff --git a/test/engine/json.test.lua b/test/engine/json.test.lua
index 4771c3162a..371bbad91d 100644
--- a/test/engine/json.test.lua
+++ b/test/engine/json.test.lua
@@ -220,9 +220,9 @@ s:insert{4, {}} -- error
 s:insert{5, {{b = 1}}} -- error
 s:insert{6, {{a = 1}}} -- ok
 s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
-s:insert{7, box.NULL} -- error
-s:insert{8, {box.NULL}} -- error
--- Skipping nullable fields is okay though.
+s:insert{7, box.NULL} -- ok
+s:insert{8, {box.NULL}} -- ok
+-- Skipping nullable fields is also okay.
 s:insert{9} -- ok
 s:insert{10, {}} -- ok
 s:insert{11, {{b = 1}}} -- ok
diff --git a/test/engine/multikey.result b/test/engine/multikey.result
index 968be4cc3b..ff72983ecd 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -794,9 +794,9 @@ _ = s:create_index('pk')
 _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
 ---
 ...
-s:insert{1, box.NULL} -- error
+s:insert{1, box.NULL} -- ok
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [1, null]
 ...
 s:insert{2, {box.NULL}} -- error
 ---
@@ -814,9 +814,9 @@ s:insert{4, {1}} -- ok
 s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}}
 ---
 ...
-s:insert{5, box.NULL} -- still error
+s:insert{5, box.NULL} -- ok
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [5, null]
 ...
 s:insert{6, {box.NULL}} -- ok
 ---
diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua
index ed70334949..653a9c9b17 100644
--- a/test/engine/multikey.test.lua
+++ b/test/engine/multikey.test.lua
@@ -214,12 +214,12 @@ s:drop()
 s = box.schema.space.create('test', {engine = engine})
 _ = s:create_index('pk')
 _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
-s:insert{1, box.NULL} -- error
+s:insert{1, box.NULL} -- ok
 s:insert{2, {box.NULL}} -- error
 s:insert{3, {}} -- ok
 s:insert{4, {1}} -- ok
 s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}}
-s:insert{5, box.NULL} -- still error
+s:insert{5, box.NULL} -- ok
 s:insert{6, {box.NULL}} -- ok
 s:insert{7, {}} -- ok
 s:insert{8, {2}} -- ok
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index

[Nikita Pettik]

On 14 Aug 17:51, Ilya Kosarev wrote:
> Multikey index did not work properly with nullable root field in
> tuple_raw_multikey_count(). Now it is fixed and corresponding
> restrictions are dropped. This also means that we can drop implicit
> nullability update for array/map fields and make all fields nullable
> by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435
> (tuple format: don't allow null where array/map is expected), as far as
> default non-nullability itself doesn't solve any real problems while
> providing confusing behavior (gh-5027).
> 
> Follow-up #5027
> Closes #5192
> 
> @TarantoolBot document
> Title: tuple: allow nullable multikey index root
> Update the documentation for space_object:create_index() to reflect
> that it is now safe for multikey index root to be nullable.
> Previously it was not safe to insert, for example, box.NULL as an array
> field which elements are the part of multikey index due to error in
> counter of multikey index keys in tuple. It was partly fixed using
> default non-nullability for tuple fields. In 2.3.3 the restriction on
> nullable multikey index root was introduced. Now the counter problem is
> fixed and all tuple fields are nullable by default as before 2.2.1.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
> Issue: https://github.com/tarantool/tarantool/issues/5192
> 
> @ChangeLog:
>  * Dropped restrictions on nullable multikey index root. They were
>  introduced due to inaccuracy in multikey index realization. It is
>  now fixed. Also all fields are now nullable by default as it was
>  before 2.2.1 (gh-5192).
> 
>  Changes in v2:
> - removed insignificant changes
> - fixed tests comments
> 
>  Changes in v3:
> - fixed some nits
> - added docbot request

LGTM now.
 

Re: [Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index

[Kirill Yukhin]

Hello,

On 14 авг 17:51, Ilya Kosarev wrote:
> Multikey index did not work properly with nullable root field in
> tuple_raw_multikey_count(). Now it is fixed and corresponding
> restrictions are dropped. This also means that we can drop implicit
> nullability update for array/map fields and make all fields nullable
> by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435
> (tuple format: don't allow null where array/map is expected), as far as
> default non-nullability itself doesn't solve any real problems while
> providing confusing behavior (gh-5027).
> 
> Follow-up #5027
> Closes #5192
> 
> @TarantoolBot document
> Title: tuple: allow nullable multikey index root
> Update the documentation for space_object:create_index() to reflect
> that it is now safe for multikey index root to be nullable.
> Previously it was not safe to insert, for example, box.NULL as an array
> field which elements are the part of multikey index due to error in
> counter of multikey index keys in tuple. It was partly fixed using
> default non-nullability for tuple fields. In 2.3.3 the restriction on
> nullable multikey index root was introduced. Now the counter problem is
> fixed and all tuple fields are nullable by default as before 2.2.1.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
> Issue: https://github.com/tarantool/tarantool/issues/5192
> 
> @ChangeLog:
>  * Dropped restrictions on nullable multikey index root. They were
>  introduced due to inaccuracy in multikey index realization. It is
>  now fixed. Also all fields are now nullable by default as it was
>  before 2.2.1 (gh-5192).

LGTM.
I've checked your patch into 2.4, 2.5 and master.

--
Regards, Kirill Yukhin