[Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
Ilya Kosarev
i.kosarev at tarantool.org
Mon Jul 13 15:36:33 MSK 2020
Since e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't
allow null where array/map is expected) tuple fields are non-nullable
by default. It seems strange at least in case we have implicit fields
in front of explicit nullable field. Also it causes incorrect behaviour
in case of using explicitly nullable array/map fields for multikey
index.
Now fields are nullable by default except arrays & maps, as far
as their implicit nullability might break field accessors expectations,
provide confusing error messages and cause incorrect behaviour of
tuple_multikey_count(). In case explicitly nullable array/map fields
are being used for multikey index, clear error message is provided.
Closes #5027
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability
Issue: https://github.com/tarantool/tarantool/issues/5027
@ChangeLog:
* Fix confusing implicit requirements for tuple fields (gh-5027).
Changes in v2:
- tuple fields nullability update is reworked & commented
- needed error messages are added
- test case is reworked & separated
src/box/memtx_space.c | 38 +++---
src/box/tuple_format.c | 22 +++-
src/box/vinyl.c | 19 ++-
test/engine/gh-5027-fields-nullability.result | 119 ++++++++++++++++++
.../gh-5027-fields-nullability.test.lua | 37 ++++++
5 files changed, 216 insertions(+), 19 deletions(-)
create mode 100644 test/engine/gh-5027-fields-nullability.result
create mode 100644 test/engine/gh-5027-fields-nullability.test.lua
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 875592026..8452ab430 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -589,7 +589,9 @@ 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)
{
- if (index_def->key_def->is_nullable) {
+ struct key_def *key_def = index_def->key_def;
+
+ if (key_def->is_nullable) {
if (index_def->iid == 0) {
diag_set(ClientError, ER_NULLABLE_PRIMARY,
space_name(space));
@@ -602,6 +604,14 @@ 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) {
@@ -610,13 +620,13 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
"HASH index must be unique");
return -1;
}
- if (index_def->key_def->is_multikey) {
+ if (key_def->is_multikey) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"HASH index cannot be multikey");
return -1;
}
- if (index_def->key_def->for_func_index) {
+ if (key_def->for_func_index) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"HASH index can not use a function");
@@ -627,7 +637,7 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
/* TREE index has no limitations. */
break;
case RTREE:
- if (index_def->key_def->part_count != 1) {
+ if (key_def->part_count != 1) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"RTREE index key can not be multipart");
@@ -639,19 +649,19 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
"RTREE index can not be unique");
return -1;
}
- if (index_def->key_def->parts[0].type != FIELD_TYPE_ARRAY) {
+ if (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 (index_def->key_def->is_multikey) {
+ if (key_def->is_multikey) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"RTREE index cannot be multikey");
return -1;
}
- if (index_def->key_def->for_func_index) {
+ if (key_def->for_func_index) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"RTREE index can not use a function");
@@ -660,7 +670,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 (index_def->key_def->part_count != 1) {
+ if (key_def->part_count != 1) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"BITSET index key can not be multipart");
@@ -672,20 +682,20 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
"BITSET can not be unique");
return -1;
}
- if (index_def->key_def->parts[0].type != FIELD_TYPE_UNSIGNED &&
- index_def->key_def->parts[0].type != FIELD_TYPE_STRING) {
+ if (key_def->parts[0].type != FIELD_TYPE_UNSIGNED &&
+ 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 (index_def->key_def->is_multikey) {
+ if (key_def->is_multikey) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"BITSET index cannot be multikey");
return -1;
}
- if (index_def->key_def->for_func_index) {
+ if (key_def->for_func_index) {
diag_set(ClientError, ER_MODIFY_INDEX,
index_def->name, space_name(space),
"BITSET index can not use a function");
@@ -700,8 +710,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 < index_def->key_def->part_count; i++) {
- struct key_part *part = &index_def->key_def->parts[i];
+ for (uint32_t i = 0; i < key_def->part_count; i++) {
+ struct key_part *part = &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_format.c b/src/box/tuple_format.c
index 68ec2a749..8cb9be8bb 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -152,7 +152,7 @@ tuple_field_new(void)
field->type = FIELD_TYPE_ANY;
field->offset_slot = TUPLE_OFFSET_SLOT_NIL;
field->coll_id = COLL_NONE;
- field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
+ field->nullable_action = ON_CONFLICT_ACTION_NONE;
field->multikey_required_fields = NULL;
return field;
}
@@ -242,6 +242,24 @@ 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
@@ -317,11 +335,13 @@ 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 fd9b7e6c0..32301d7ba 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -651,13 +651,24 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
index_def->name, space_name(space));
return -1;
}
- if (index_def->key_def->is_nullable && index_def->iid == 0) {
+
+ struct key_def *key_def = index_def->key_def;
+
+ if (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 < index_def->key_def->part_count; i++) {
- struct key_part *part = &index_def->key_def->parts[i];
+ for (uint32_t i = 0; i < key_def->part_count; i++) {
+ struct key_part *part = &key_def->parts[i];
if (part->type <= FIELD_TYPE_ANY ||
part->type >= FIELD_TYPE_ARRAY) {
diag_set(ClientError, ER_MODIFY_INDEX,
@@ -667,7 +678,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
return -1;
}
}
- if (index_def->key_def->for_func_index) {
+ if (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-fields-nullability.result
new file mode 100644
index 000000000..1121727f6
--- /dev/null
+++ b/test/engine/gh-5027-fields-nullability.result
@@ -0,0 +1,119 @@
+-- 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:create_index('i1', {parts={{1, 'unsigned'}}})
+ | ---
+ | ...
+_ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
+ | ---
+ | ...
+s:replace{1}
+ | ---
+ | - [1]
+ | ...
+s:replace{1, box.NULL}
+ | ---
+ | - [1, null]
+ | ...
+s:replace{1, box.NULL, box.NULL}
+ | ---
+ | - [1, null, null]
+ | ...
+s:replace{1, box.NULL, box.NULL, box.NULL}
+ | ---
+ | - [1, null, null, null]
+ | ...
+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}
+ | ---
+ | - error: Tuple field 5 required by space format is missing
+ | ...
+s:replace{1, box.NULL}
+ | ---
+ | - error: Tuple field 5 required by space format is missing
+ | ...
+s:replace{1, box.NULL, box.NULL}
+ | ---
+ | - error: Tuple field 5 required by space format is missing
+ | ...
+s:replace{1, box.NULL, box.NULL, box.NULL}
+ | ---
+ | - error: Tuple field 5 required by space format is missing
+ | ...
+s:replace{1, box.NULL, box.NULL, box.NULL, 5}
+ | ---
+ | - [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
new file mode 100644
index 000000000..960103d6c
--- /dev/null
+++ b/test/engine/gh-5027-fields-nullability.test.lua
@@ -0,0 +1,37 @@
+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: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:drop()
--
2.17.1
More information about the Tarantool-patches
mailing list