Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index
@ 2020-08-14 14:51 Ilya Kosarev
  2020-08-21  8:46 ` Nikita Pettik
  2020-08-25  9:42 ` Kirill Yukhin
  0 siblings, 2 replies; 3+ messages in thread
From: Ilya Kosarev @ 2020-08-14 14:51 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

@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

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

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14 14:51 [Tarantool-patches] [PATCH v3] tuple: drop extra restrictions for multikey index Ilya Kosarev
2020-08-21  8:46 ` Nikita Pettik
2020-08-25  9:42 ` Kirill Yukhin

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