Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions for multikey index
@ 2020-08-05 10:12 Ilya Kosarev
  2020-08-05 12:29 ` Nikita Pettik
  0 siblings, 1 reply; 3+ messages in thread
From: Ilya Kosarev @ 2020-08-05 10:12 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches

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

@ChangeLog:
 * Dropped restrictions on nullable multikey index root. 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

 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 | 71 +++----------------
 .../gh-5027-fields-nullability.test.lua       | 35 +++------
 .../gh-5192-multikey-root-nullability.result  | 62 ++++++++++++++++
 ...gh-5192-multikey-root-nullability.test.lua | 20 ++++++
 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, 123 insertions(+), 144 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 8452ab430..d30ce44b8 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 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..5fa3ea3a5 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 1121727f6..382944d53 100644
--- a/test/engine/gh-5027-fields-nullability.result
+++ b/test/engine/gh-5027-fields-nullability.result
@@ -12,19 +12,19 @@ _ = s:create_index('i1', {parts={{1, 'unsigned'}}})
 _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
  | ---
  | ...
-s:replace{1}
+s:replace{1} -- ok
  | ---
  | - [1]
  | ...
-s:replace{1, box.NULL}
+s:replace{1, box.NULL} -- ok
  | ---
  | - [1, null]
  | ...
-s:replace{1, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL} -- ok
  | ---
  | - [1, null, null]
  | ...
-s:replace{1, box.NULL, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL, box.NULL} -- ok
  | ---
  | - [1, null, null, null]
  | ...
@@ -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 960103d6c..664883d0d 100644
--- a/test/engine/gh-5027-fields-nullability.test.lua
+++ b/test/engine/gh-5027-fields-nullability.test.lua
@@ -3,35 +3,18 @@ test_run = require('test_run').new()
 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=true}}})
-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} -- ok
+s:replace{1, box.NULL} -- ok
+s:replace{1, box.NULL, box.NULL} -- ok
+s:replace{1, box.NULL, box.NULL, box.NULL} -- ok
 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 000000000..dcbb5067f
--- /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} -- 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: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
+ | ---
+ | - 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 000000000..5a8305bf9
--- /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} -- ok
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- ok
+_ = 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
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- error
+s:replace{3, {}} -- ok
+s:drop()
\ No newline at end of file
diff --git a/test/engine/json.result b/test/engine/json.result
index 2175266fc..b8fd9a1b6 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 4771c3162..371bbad91 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 968be4cc3..ff72983ec 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 ed7033494..653a9c9b1 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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions for multikey index
  2020-08-05 10:12 [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions for multikey index Ilya Kosarev
@ 2020-08-05 12:29 ` Nikita Pettik
  0 siblings, 0 replies; 3+ messages in thread
From: Nikita Pettik @ 2020-08-05 12:29 UTC (permalink / raw)
  To: Ilya Kosarev; +Cc: tarantool-patches

On 05 Aug 13:12, Ilya Kosarev wrote:
> @ChangeLog:
>  * Dropped restrictions on nullable multikey index root. All fields are
>  now nullable by default as it was before 2.2.1 (gh-5192).

I'd underline the fact that problem was fixed in another (local code
change) way.

What is more, I guess we should document this behaviour: our current
docs say nothing concerning nulls as multikey roots.
 
> diff --git a/test/engine/gh-5027-fields-nullability.test.lua b/test/engine/gh-5027-fields-nullability.test.lua
> index 960103d6c..664883d0d 100644
> --- a/test/engine/gh-5027-fields-nullability.test.lua
> +++ b/test/engine/gh-5027-fields-nullability.test.lua
> @@ -3,35 +3,18 @@ test_run = require('test_run').new()
>  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=true}}})
> -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} -- ok
> +s:replace{1, box.NULL} -- ok
> +s:replace{1, box.NULL, box.NULL} -- ok
> +s:replace{1, box.NULL, box.NULL, box.NULL} -- ok

I'd drop these -- ok comments since they only enlarge diff.

> 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..5a8305bf9
> --- /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} -- ok
> +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
> +s:replace{2, box.NULL} -- ok
> +_ = s:delete(2)
> +s:truncate()

Why do you need to truncate space before drop?

> +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()
> \ No newline at end of file

Nit: put pls newline at the end of file to avoid this warnings.

> diff --git a/test/engine/json.test.lua b/test/engine/json.test.lua
> index 4771c3162..371bbad91 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.

Is this change really required? :)

>  s:insert{9} -- ok
>  s:insert{10, {}} -- ok
>  s:insert{11, {{b = 1}}} -- ok

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions for multikey index
@ 2020-08-14 10:41 Ilya Kosarev
  0 siblings, 0 replies; 3+ messages in thread
From: Ilya Kosarev @ 2020-08-14 10:41 UTC (permalink / raw)
  To: alyapunov; +Cc: tarantool-patches

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

@ChangeLog:
 * Dropped restrictions on nullable multikey index root. 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

 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 | 71 +++----------------
 .../gh-5027-fields-nullability.test.lua       | 35 +++------
 .../gh-5192-multikey-root-nullability.result  | 62 ++++++++++++++++
 ...gh-5192-multikey-root-nullability.test.lua | 20 ++++++
 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, 123 insertions(+), 144 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 8452ab430..d30ce44b8 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 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..5fa3ea3a5 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 1121727f6..382944d53 100644
--- a/test/engine/gh-5027-fields-nullability.result
+++ b/test/engine/gh-5027-fields-nullability.result
@@ -12,19 +12,19 @@ _ = s:create_index('i1', {parts={{1, 'unsigned'}}})
 _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
  | ---
  | ...
-s:replace{1}
+s:replace{1} -- ok
  | ---
  | - [1]
  | ...
-s:replace{1, box.NULL}
+s:replace{1, box.NULL} -- ok
  | ---
  | - [1, null]
  | ...
-s:replace{1, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL} -- ok
  | ---
  | - [1, null, null]
  | ...
-s:replace{1, box.NULL, box.NULL, box.NULL}
+s:replace{1, box.NULL, box.NULL, box.NULL} -- ok
  | ---
  | - [1, null, null, null]
  | ...
@@ -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 960103d6c..664883d0d 100644
--- a/test/engine/gh-5027-fields-nullability.test.lua
+++ b/test/engine/gh-5027-fields-nullability.test.lua
@@ -3,35 +3,18 @@ test_run = require('test_run').new()
 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=true}}})
-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} -- ok
+s:replace{1, box.NULL} -- ok
+s:replace{1, box.NULL, box.NULL} -- ok
+s:replace{1, box.NULL, box.NULL, box.NULL} -- ok
 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 000000000..dcbb5067f
--- /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} -- 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: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
+ | ---
+ | - 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 000000000..5a8305bf9
--- /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} -- ok
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- ok
+_ = 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
+_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
+s:replace{2, box.NULL} -- error
+s:replace{3, {}} -- ok
+s:drop()
\ No newline at end of file
diff --git a/test/engine/json.result b/test/engine/json.result
index 2175266fc..b8fd9a1b6 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 4771c3162..371bbad91 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 968be4cc3..ff72983ec 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 ed7033494..653a9c9b1 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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-14 10:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05 10:12 [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions for multikey index Ilya Kosarev
2020-08-05 12:29 ` Nikita Pettik
2020-08-14 10:41 Ilya Kosarev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox