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

[Tarantool-patches] [PATCH] 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
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
Issue: https://github.com/tarantool/tarantool/issues/5192

 src/box/memtx_space.c                         | 38 +++++-------
 src/box/tuple.c                               |  5 +-
 src/box/tuple_format.c                        | 20 ------
 src/box/vinyl.c                               | 19 ++----
 ...> gh-5027-extra-fields-nullability.result} | 53 ----------------
 ...gh-5027-extra-fields-nullability.test.lua} | 17 -----
 .../gh-5192-multikey-root-nullability.result  | 62 +++++++++++++++++++
 ...gh-5192-multikey-root-nullability.test.lua | 20 ++++++
 test/engine/json.result                       | 14 ++---
 test/engine/multikey.result                   |  4 +-
 10 files changed, 113 insertions(+), 139 deletions(-)
 rename test/engine/{gh-5027-fields-nullability.result => gh-5027-extra-fields-nullability.result} (53%)
 rename test/engine/{gh-5027-fields-nullability.test.lua => gh-5027-extra-fields-nullability.test.lua} (51%)
 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 8452ab430..875592026 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -589,9 +589,7 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
 static int
 memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 {
-	struct key_def *key_def = index_def->key_def;
-
-	if (key_def->is_nullable) {
+	if (index_def->key_def->is_nullable) {
 		if (index_def->iid == 0) {
 			diag_set(ClientError, ER_NULLABLE_PRIMARY,
 				 space_name(space));
@@ -604,14 +602,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) {
@@ -620,13 +610,13 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 				 "HASH index must be unique");
 			return -1;
 		}
-		if (key_def->is_multikey) {
+		if (index_def->key_def->is_multikey) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "HASH index cannot be multikey");
 			return -1;
 		}
-		if (key_def->for_func_index) {
+		if (index_def->key_def->for_func_index) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "HASH index can not use a function");
@@ -637,7 +627,7 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 		/* TREE index has no limitations. */
 		break;
 	case RTREE:
-		if (key_def->part_count != 1) {
+		if (index_def->key_def->part_count != 1) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "RTREE index key can not be multipart");
@@ -649,19 +639,19 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 				 "RTREE index can not be unique");
 			return -1;
 		}
-		if (key_def->parts[0].type != FIELD_TYPE_ARRAY) {
+		if (index_def->key_def->parts[0].type != FIELD_TYPE_ARRAY) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "RTREE index field type must be ARRAY");
 			return -1;
 		}
-		if (key_def->is_multikey) {
+		if (index_def->key_def->is_multikey) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "RTREE index cannot be multikey");
 			return -1;
 		}
-		if (key_def->for_func_index) {
+		if (index_def->key_def->for_func_index) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "RTREE index can not use a function");
@@ -670,7 +660,7 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 		/* no furter checks of parts needed */
 		return 0;
 	case BITSET:
-		if (key_def->part_count != 1) {
+		if (index_def->key_def->part_count != 1) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "BITSET index key can not be multipart");
@@ -682,20 +672,20 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 				 "BITSET can not be unique");
 			return -1;
 		}
-		if (key_def->parts[0].type != FIELD_TYPE_UNSIGNED &&
-		    key_def->parts[0].type != FIELD_TYPE_STRING) {
+		if (index_def->key_def->parts[0].type != FIELD_TYPE_UNSIGNED &&
+		    index_def->key_def->parts[0].type != FIELD_TYPE_STRING) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "BITSET index field type must be NUM or STR");
 			return -1;
 		}
-		if (key_def->is_multikey) {
+		if (index_def->key_def->is_multikey) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "BITSET index cannot be multikey");
 			return -1;
 		}
-		if (key_def->for_func_index) {
+		if (index_def->key_def->for_func_index) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
 				 index_def->name, space_name(space),
 				 "BITSET index can not use a function");
@@ -710,8 +700,8 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 	}
 	/* Only HASH and TREE indexes checks parts there */
 	/* 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];
+	for (uint32_t i = 0; i < index_def->key_def->part_count; i++) {
+		struct key_part *part = &index_def->key_def->parts[i];
 		if (part->type <= FIELD_TYPE_ANY ||
 		    part->type >= FIELD_TYPE_ARRAY) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
diff --git a/src/box/tuple.c b/src/box/tuple.c
index 9f0f24c64..94a3a96ba 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -554,7 +554,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 bae6c67cd..9b817d3cf 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 32301d7ba..fd9b7e6c0 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -651,24 +651,13 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
 			 index_def->name, space_name(space));
 		return -1;
 	}
-
-	struct key_def *key_def = index_def->key_def;
-
-	if (key_def->is_nullable && index_def->iid == 0) {
+	if (index_def->key_def->is_nullable && index_def->iid == 0) {
 		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];
+	for (uint32_t i = 0; i < index_def->key_def->part_count; i++) {
+		struct key_part *part = &index_def->key_def->parts[i];
 		if (part->type <= FIELD_TYPE_ANY ||
 		    part->type >= FIELD_TYPE_ARRAY) {
 			diag_set(ClientError, ER_MODIFY_INDEX,
@@ -678,7 +667,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
 			return -1;
 		}
 	}
-	if (key_def->for_func_index) {
+	if (index_def->key_def->for_func_index) {
 		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
 			 "functional index");
 		return -1;
diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-extra-fields-nullability.result
similarity index 53%
rename from test/engine/gh-5027-fields-nullability.result
rename to test/engine/gh-5027-extra-fields-nullability.result
index 1121727f6..372fe15b8 100644
--- a/test/engine/gh-5027-fields-nullability.result
+++ b/test/engine/gh-5027-extra-fields-nullability.result
@@ -64,56 +64,3 @@ 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}
- | ---
- | - [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-extra-fields-nullability.test.lua
similarity index 51%
rename from test/engine/gh-5027-fields-nullability.test.lua
rename to test/engine/gh-5027-extra-fields-nullability.test.lua
index 960103d6c..5d72d949a 100644
--- a/test/engine/gh-5027-fields-nullability.test.lua
+++ b/test/engine/gh-5027-extra-fields-nullability.test.lua
@@ -18,20 +18,3 @@ 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: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 000000000..84a94e2b3
--- /dev/null
+++ b/test/engine/gh-5192-multikey-root-nullability.result
@@ -0,0 +1,62 @@
+-- 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}
+ | ---
+ | - [1, null]
+ | ...
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+ | ---
+ | ...
+s:replace{2, box.NULL}
+ | ---
+ | - [2, null]
+ | ...
+_ = s:delete(2)
+ | ---
+ | ...
+s:truncate()
+ | ---
+ | ...
+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-5192-multikey-root-nullability.test.lua b/test/engine/gh-5192-multikey-root-nullability.test.lua
new file mode 100644
index 000000000..8a4c45fb8
--- /dev/null
+++ b/test/engine/gh-5192-multikey-root-nullability.test.lua
@@ -0,0 +1,20 @@
+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}
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL}
+_ = s:delete(2)
+s:truncate()
+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:drop()
\ No newline at end of file
diff --git a/test/engine/json.result b/test/engine/json.result
index 2175266fc..bc1d7cad5 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
 ---
@@ -766,12 +765,11 @@ s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
 ...
 s:insert{7, box.NULL} -- error
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [7, null]
 ...
 s:insert{8, {box.NULL}} -- error
 ---
-- error: 'Tuple field [2][1] type does not match one required by operation: expected
-    map'
+- [8, [null]]
 ...
 -- Skipping nullable fields is okay though.
 s:insert{9} -- ok
@@ -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/multikey.result b/test/engine/multikey.result
index 968be4cc3..e9005743e 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -796,7 +796,7 @@ _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
 ...
 s:insert{1, box.NULL} -- error
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [1, null]
 ...
 s:insert{2, {box.NULL}} -- error
 ---
@@ -816,7 +816,7 @@ s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}}
 ...
 s:insert{5, box.NULL} -- still error
 ---
-- error: 'Tuple field 2 type does not match one required by operation: expected array'
+- [5, null]
 ...
 s:insert{6, {box.NULL}} -- ok
 ---
-- 
2.17.1

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

[Nikita Pettik]

On 24 Jul 23:06, 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
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
> Issue: https://github.com/tarantool/tarantool/issues/5192
> 

I *strongly dislike* that the patch is in fact should be 10 lines
but instead we have 250 diff lines. Please elaborate on that
(comments above are mostly about that).

> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 8452ab430..875592026 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -589,9 +589,7 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
>  static int
>  memtx_space_check_index_def(struct space *space, struct index_def *index_def)
>  {
> -	struct key_def *key_def = index_def->key_def;
> -
> -	if (key_def->is_nullable) {
> +	if (index_def->key_def->is_nullable) {

Again I see this redundant diff. Please drop it, it doesn't fix
refactor/fix anything but makes diff bigger and complicates git
history.

>  		if (index_def->iid == 0) {
>  			diag_set(ClientError, ER_NULLABLE_PRIMARY,
>  				 space_name(space));
> diff --git a/src/box/tuple.c b/src/box/tuple.c
> index 9f0f24c64..94a3a96ba 100644
> --- a/src/box/tuple.c
> +++ b/src/box/tuple.c
> @@ -554,7 +554,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);
>  }
>  
> @@ -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 32301d7ba..fd9b7e6c0 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -651,24 +651,13 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
>  			 index_def->name, space_name(space));
>  		return -1;
>  	}
> -
> -	struct key_def *key_def = index_def->key_def;
> -
> -	if (key_def->is_nullable && index_def->iid == 0) {
> +	if (index_def->key_def->is_nullable && index_def->iid == 0) {
>  		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];
> +	for (uint32_t i = 0; i < index_def->key_def->part_count; i++) {
> +		struct key_part *part = &index_def->key_def->parts[i];
>  		if (part->type <= FIELD_TYPE_ANY ||
>  		    part->type >= FIELD_TYPE_ARRAY) {
>  			diag_set(ClientError, ER_MODIFY_INDEX,
> @@ -678,7 +667,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
>  			return -1;
>  		}
>  	}
> -	if (key_def->for_func_index) {
> +	if (index_def->key_def->for_func_index) {
>  		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
>  			 "functional index");
>  		return -1;
> diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-extra-fields-nullability.result
> similarity index 53%
> rename from test/engine/gh-5027-fields-nullability.result
> rename to test/engine/gh-5027-extra-fields-nullability.result

Please, avoid renaming test..

> index 1121727f6..372fe15b8 100644
> diff --git a/test/engine/multikey.result b/test/engine/multikey.result
> index 968be4cc3..e9005743e 100644
> --- a/test/engine/multikey.result
> +++ b/test/engine/multikey.result
> @@ -796,7 +796,7 @@ _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
>  ...
>  s:insert{1, box.NULL} -- error

I guess you forgot to fix this comment (and the rest in this test)...
 

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

[Ilya Kosarev]

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


Hi!
 
Thanks for your review.
 
See my answers below.
 
I will send v2 of the patch but i think 2 questions above should be clarified.
  
>Четверг, 30 июля 2020, 17:26 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> 
>On 24 Jul 23:06, 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
>> ---
>> Branch:  https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions
>> Issue:  https://github.com/tarantool/tarantool/issues/5192
>>
>
>I *strongly dislike* that the patch is in fact should be 10 lines
>but instead we have 250 diff lines. Please elaborate on that
>(comments above are mostly about that).
Well, this patch reverts the majority of changes from
4cf94ef8cb90b84ea71f313cff3e016f85894fd5 (tuple: make fields nullable
by default except array/map). That is where the size comes from.
>
>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
>> index 8452ab430..875592026 100644
>> --- a/src/box/memtx_space.c
>> +++ b/src/box/memtx_space.c
>> @@ -589,9 +589,7 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
>> static int
>> memtx_space_check_index_def(struct space *space, struct index_def *index_def)
>> {
>> - struct key_def *key_def = index_def->key_def;
>> -
>> - if (key_def->is_nullable) {
>> + if (index_def->key_def->is_nullable) {
>
>Again I see this redundant diff. Please drop it, it doesn't fix
>refactor/fix anything but makes diff bigger and complicates git
>history.
The problem is that this is the reversion of that diff. Well, I guess
as far as that patch was pushed to master we can leave that change
there and revert only the meaningful part?
>
>> if (index_def->iid == 0) {
>> diag_set(ClientError, ER_NULLABLE_PRIMARY,
>> space_name(space));
>> diff --git a/src/box/tuple.c b/src/box/tuple.c
>> index 9f0f24c64..94a3a96ba 100644
>> --- a/src/box/tuple.c
>> +++ b/src/box/tuple.c
>> @@ -554,7 +554,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);
>> }
>>
>> @@ -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 32301d7ba..fd9b7e6c0 100644
>> --- a/src/box/vinyl.c
>> +++ b/src/box/vinyl.c
>> @@ -651,24 +651,13 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
>> index_def->name, space_name(space));
>> return -1;
>> }
>> -
>> - struct key_def *key_def = index_def->key_def;
>> -
>> - if (key_def->is_nullable && index_def->iid == 0) {
>> + if (index_def->key_def->is_nullable && index_def->iid == 0) {
>> 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];
>> + for (uint32_t i = 0; i < index_def->key_def->part_count; i++) {
>> + struct key_part *part = &index_def->key_def->parts[i];
>> if (part->type <= FIELD_TYPE_ANY ||
>> part->type >= FIELD_TYPE_ARRAY) {
>> diag_set(ClientError, ER_MODIFY_INDEX,
>> @@ -678,7 +667,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
>> return -1;
>> }
>> }
>> - if (key_def->for_func_index) {
>> + if (index_def->key_def->for_func_index) {
>> diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
>> "functional index");
>> return -1;
>> diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-extra-fields-nullability.result
>> similarity index 53%
>> rename from test/engine/gh-5027-fields-nullability.result
>> rename to test/engine/gh-5027-extra-fields-nullability.result
>
>Please, avoid renaming test..
I guess we can drop this change, however i thought it is a good idea
to make test name more clear, as you proposed:
>>Nit: mb gh-5027-extra-fields-nullability or gh-5027-compound-field-type-nullability?
>>Then others would understand what this test is about with ease.
Is it really that bad to rename?
>
>> index 1121727f6..372fe15b8 100644
>> diff --git a/test/engine/multikey.result b/test/engine/multikey.result
>> index 968be4cc3..e9005743e 100644
>> --- a/test/engine/multikey.result
>> +++ b/test/engine/multikey.result
>> @@ -796,7 +796,7 @@ _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
>> ...
>> s:insert{1, box.NULL} -- error
>
>I guess you forgot to fix this comment (and the rest in this test)…
Right, this comments have to be fixed.
 
--
Ilya Kosarev
 

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

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

[Nikita Pettik]

On 31 Jul 16:13, Ilya Kosarev wrote:
> 
> Hi!
>  
> Thanks for your review.
>  
> See my answers below.
>  
> I will send v2 of the patch but i think 2 questions above should be clarified.
>   
> >Четверг, 30 июля 2020, 17:26 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> > 
> >On 24 Jul 23:06, Ilya Kosarev wrote:
> >
> >I *strongly dislike* that the patch is in fact should be 10 lines
> >but instead we have 250 diff lines. Please elaborate on that
> >(comments above are mostly about that).
> Well, this patch reverts the majority of changes from
> 4cf94ef8cb90b84ea71f313cff3e016f85894fd5 (tuple: make fields nullable
> by default except array/map). That is where the size comes from.

Then you are able simply to revert that patch and introduce new one.
But I'd provide only required fixes.

> >> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> >> index 8452ab430..875592026 100644
> >> --- a/src/box/memtx_space.c
> >> +++ b/src/box/memtx_space.c
> >> @@ -589,9 +589,7 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)
> >> static int
> >> memtx_space_check_index_def(struct space *space, struct index_def *index_def)
> >> {
> >> - struct key_def *key_def = index_def->key_def;
> >> -
> >> - if (key_def->is_nullable) {
> >> + if (index_def->key_def->is_nullable) {
> >
> >Again I see this redundant diff. Please drop it, it doesn't fix
> >refactor/fix anything but makes diff bigger and complicates git
> >history.
> The problem is that this is the reversion of that diff. Well, I guess
> as far as that patch was pushed to master we can leave that change
> there and revert only the meaningful part?

Yes.