From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E14D5445320 for ; Sun, 12 Jul 2020 16:43:21 +0300 (MSK) From: Ilya Kosarev Date: Sun, 12 Jul 2020 16:43:16 +0300 Message-Id: <20200712134316.31126-1-i.kosarev@tarantool.org> Subject: [Tarantool-patches] [PATCH v2] tuple: make fields nullable by default except array/map List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org 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