* [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
@ 2020-07-12 13:43 Ilya Kosarev
2020-07-13 11:19 ` Aleksandr Lyapunov
2020-07-13 13:38 ` Kirill Yukhin
0 siblings, 2 replies; 7+ messages in thread
From: Ilya Kosarev @ 2020-07-12 13:43 UTC (permalink / raw)
To: tarantool-patches
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.
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
2020-07-12 13:43 [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map Ilya Kosarev
@ 2020-07-13 11:19 ` Aleksandr Lyapunov
2020-07-13 13:38 ` Kirill Yukhin
1 sibling, 0 replies; 7+ messages in thread
From: Aleksandr Lyapunov @ 2020-07-13 11:19 UTC (permalink / raw)
To: Ilya Kosarev, tarantool-patches
Hi! thanks for the patch! great job, lgtm now.
On 12.07.2020 16:43, Ilya Kosarev wrote:
> 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.
>
> 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()
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
2020-07-12 13:43 [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map Ilya Kosarev
2020-07-13 11:19 ` Aleksandr Lyapunov
@ 2020-07-13 13:38 ` Kirill Yukhin
1 sibling, 0 replies; 7+ messages in thread
From: Kirill Yukhin @ 2020-07-13 13:38 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
Hello,
On 12 июл 16:43, Ilya Kosarev wrote:
> 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
LGTM.
I've checked your patch into 2.3, 2.4 and master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
2020-07-14 12:37 ` Nikita Pettik
@ 2020-07-14 12:54 ` Ilya Kosarev
0 siblings, 0 replies; 7+ messages in thread
From: Ilya Kosarev @ 2020-07-14 12:54 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
Hi!
Thanks for the review.
Right, i will consider your comments
( https://lists.tarantool.org/pipermail/tarantool-patches/2020-July/018525.html )
and provide corresponding follow-up.
>Вторник, 14 июля 2020, 15:37 +03:00 от Nikita Pettik <korablev@tarantool.org>:
>
>On 14 Jul 10:37, Nikita Pettik wrote:
>
>I see that Kirill accidentally (at least I hope so) pushed it
>to master without getting the second approvement. So I suggest
>you to provide fixes (part of them for sure now makes less sense)
>as follow-up.
--
Ilya Kosarev
[-- Attachment #2: Type: text/html, Size: 1144 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
2020-07-14 10:37 ` Nikita Pettik
@ 2020-07-14 12:37 ` Nikita Pettik
2020-07-14 12:54 ` Ilya Kosarev
0 siblings, 1 reply; 7+ messages in thread
From: Nikita Pettik @ 2020-07-14 12:37 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
On 14 Jul 10:37, Nikita Pettik wrote:
I see that Kirill accidentally (at least I hope so) pushed it
to master without getting the second approvement. So I suggest
you to provide fixes (part of them for sure now makes less sense)
as follow-up.
> On 13 Jul 15:36, Ilya Kosarev wrote:
> > 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
> > ---
> > 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;
>
> Hm, this change results in redundant diff below in this function.
> Mb better get rid of it?
>
> > + 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;
> > + }
>
> Consider refactoring:
>
> @@ -607,9 +606,8 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
> 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");
> + diag_set(ClientError, ER_UNSUPPORTED, "multikey index",
> + "nullable root field");
> return -1;
> }
>
> Moreover, I suggest moving this check to a separate patch.
> See comments below.
>
> > 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
>
> Is it only about _count() func? I mean mb it is easier to fix
> that function, than update nullability during format creation?
>
> > + * 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;
> > +}
>
> I don't like much name of this function...
> Consider these variants:
> tuple_field_set_composite_nullability()
> tuple_field_update_composite_nullability()
> tuple_field_update_compound_type_nullability()
> ...
>
> Or smth like that
>
>
> > /**
> > * 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);
>
> I've commented this func call and all tests have been passed.
> Please investigate why and either remove this call or provide
> corresponding test.
>
> > /*
> > * 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;
>
> Ditto: don't enlarge diff by introducing new var - it's not
> reasonable here (IMHO).
>
> > + 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;
> > + }
>
> Same check. Mb worth moving it to a helper? At least let's format
> it properly.
>
> > /* 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
>
> 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.
>
> > 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
> > 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()
>
> As I see these two cases are a bit different. In fact in your patch
> you fix two independent problems. I suggest you to split them into
> two separate patches/tests. Or provide solid arguments why we shouldn't
> do so.
>
> > +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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
2020-07-13 12:36 Ilya Kosarev
@ 2020-07-14 10:37 ` Nikita Pettik
2020-07-14 12:37 ` Nikita Pettik
0 siblings, 1 reply; 7+ messages in thread
From: Nikita Pettik @ 2020-07-14 10:37 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
On 13 Jul 15:36, Ilya Kosarev wrote:
> 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
> ---
> 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;
Hm, this change results in redundant diff below in this function.
Mb better get rid of it?
> + 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;
> + }
Consider refactoring:
@@ -607,9 +606,8 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
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");
+ diag_set(ClientError, ER_UNSUPPORTED, "multikey index",
+ "nullable root field");
return -1;
}
Moreover, I suggest moving this check to a separate patch.
See comments below.
> 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
Is it only about _count() func? I mean mb it is easier to fix
that function, than update nullability during format creation?
> + * 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;
> +}
I don't like much name of this function...
Consider these variants:
tuple_field_set_composite_nullability()
tuple_field_update_composite_nullability()
tuple_field_update_compound_type_nullability()
...
Or smth like that
> /**
> * 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);
I've commented this func call and all tests have been passed.
Please investigate why and either remove this call or provide
corresponding test.
> /*
> * 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;
Ditto: don't enlarge diff by introducing new var - it's not
reasonable here (IMHO).
> + 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;
> + }
Same check. Mb worth moving it to a helper? At least let's format
it properly.
> /* 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
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.
> 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
> 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()
As I see these two cases are a bit different. In fact in your patch
you fix two independent problems. I suggest you to split them into
two separate patches/tests. Or provide solid arguments why we shouldn't
do so.
> +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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map
@ 2020-07-13 12:36 Ilya Kosarev
2020-07-14 10:37 ` Nikita Pettik
0 siblings, 1 reply; 7+ messages in thread
From: Ilya Kosarev @ 2020-07-13 12:36 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-14 12:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-12 13:43 [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map Ilya Kosarev
2020-07-13 11:19 ` Aleksandr Lyapunov
2020-07-13 13:38 ` Kirill Yukhin
2020-07-13 12:36 Ilya Kosarev
2020-07-14 10:37 ` Nikita Pettik
2020-07-14 12:37 ` Nikita Pettik
2020-07-14 12:54 ` Ilya Kosarev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox