From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 83786445320 for ; Fri, 24 Jul 2020 23:06:38 +0300 (MSK) From: Ilya Kosarev Date: Fri, 24 Jul 2020 23:06:33 +0300 Message-Id: <20200724200633.12122-1-i.kosarev@tarantool.org> Subject: [Tarantool-patches] [PATCH] tuple: drop extra restrictions for multikey index List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: korablev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Multikey index did not work properly with nullable root field in tuple_raw_multikey_count(). Now it is fixed and corresponding restrictions are dropped. This also means that we can drop implicit nullability update for array/map fields and make all fields nullable by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't allow null where array/map is expected), as far as default non-nullability itself doesn't solve any real problems while providing confusing behavior (gh-5027). Follow-up #5027 Closes #5192 --- Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions Issue: https://github.com/tarantool/tarantool/issues/5192 src/box/memtx_space.c | 38 +++++------- src/box/tuple.c | 5 +- src/box/tuple_format.c | 20 ------ src/box/vinyl.c | 19 ++---- ...> gh-5027-extra-fields-nullability.result} | 53 ---------------- ...gh-5027-extra-fields-nullability.test.lua} | 17 ----- .../gh-5192-multikey-root-nullability.result | 62 +++++++++++++++++++ ...gh-5192-multikey-root-nullability.test.lua | 20 ++++++ test/engine/json.result | 14 ++--- test/engine/multikey.result | 4 +- 10 files changed, 113 insertions(+), 139 deletions(-) rename test/engine/{gh-5027-fields-nullability.result => gh-5027-extra-fields-nullability.result} (53%) rename test/engine/{gh-5027-fields-nullability.test.lua => gh-5027-extra-fields-nullability.test.lua} (51%) create mode 100644 test/engine/gh-5192-multikey-root-nullability.result create mode 100644 test/engine/gh-5192-multikey-root-nullability.test.lua diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 8452ab430..875592026 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -589,9 +589,7 @@ 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) { - struct key_def *key_def = index_def->key_def; - - if (key_def->is_nullable) { + if (index_def->key_def->is_nullable) { if (index_def->iid == 0) { diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name(space)); @@ -604,14 +602,6 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) return -1; } } - if (key_def->is_multikey && - key_def->multikey_fieldno < space->def->field_count && - space->def->fields[key_def->multikey_fieldno].is_nullable) { - diag_set(ClientError, ER_UNSUPPORTED, - "multikey index", - "nullable root field"); - return -1; - } switch (index_def->type) { case HASH: if (! index_def->opts.is_unique) { @@ -620,13 +610,13 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) "HASH index must be unique"); return -1; } - if (key_def->is_multikey) { + if (index_def->key_def->is_multikey) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "HASH index cannot be multikey"); return -1; } - if (key_def->for_func_index) { + if (index_def->key_def->for_func_index) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "HASH index can not use a function"); @@ -637,7 +627,7 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) /* TREE index has no limitations. */ break; case RTREE: - if (key_def->part_count != 1) { + if (index_def->key_def->part_count != 1) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "RTREE index key can not be multipart"); @@ -649,19 +639,19 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) "RTREE index can not be unique"); return -1; } - if (key_def->parts[0].type != FIELD_TYPE_ARRAY) { + if (index_def->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 (key_def->is_multikey) { + if (index_def->key_def->is_multikey) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "RTREE index cannot be multikey"); return -1; } - if (key_def->for_func_index) { + if (index_def->key_def->for_func_index) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "RTREE index can not use a function"); @@ -670,7 +660,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 (key_def->part_count != 1) { + if (index_def->key_def->part_count != 1) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "BITSET index key can not be multipart"); @@ -682,20 +672,20 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def) "BITSET can not be unique"); return -1; } - if (key_def->parts[0].type != FIELD_TYPE_UNSIGNED && - key_def->parts[0].type != FIELD_TYPE_STRING) { + if (index_def->key_def->parts[0].type != FIELD_TYPE_UNSIGNED && + index_def->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 (key_def->is_multikey) { + if (index_def->key_def->is_multikey) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "BITSET index cannot be multikey"); return -1; } - if (key_def->for_func_index) { + if (index_def->key_def->for_func_index) { diag_set(ClientError, ER_MODIFY_INDEX, index_def->name, space_name(space), "BITSET index can not use a function"); @@ -710,8 +700,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 < key_def->part_count; i++) { - struct key_part *part = &key_def->parts[i]; + for (uint32_t i = 0; i < index_def->key_def->part_count; i++) { + struct key_part *part = &index_def->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.c b/src/box/tuple.c index 9f0f24c64..94a3a96ba 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -554,7 +554,10 @@ tuple_raw_multikey_count(struct tuple_format *format, const char *data, NULL, MULTIKEY_NONE); if (array_raw == NULL) return 0; - assert(mp_typeof(*array_raw) == MP_ARRAY); + enum mp_type type = mp_typeof(*array_raw); + if (type == MP_NIL) + return 0; + assert(type == MP_ARRAY); return mp_decode_array(&array_raw); } diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index bae6c67cd..9b817d3cf 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -242,24 +242,6 @@ tuple_field_ensure_child_compatibility(struct tuple_field *parent, return 0; } -/** - * Tuple fields are nullable by default. However, it is not ok - * for array/map fields, as far as their implicit nullability - * might break field accessors expectations, provide confusing - * error messages and cause incorrect behaviour of - * tuple_multikey_count(). Thus array/map fields have to be - * non-nullable by default, which means we have to update default - * nullability for them. - */ -static void -tuple_field_update_nullability(struct tuple_field *field) -{ - if ((field->type == FIELD_TYPE_ARRAY || - field->type == FIELD_TYPE_MAP) && - tuple_field_is_nullable(field)) - field->nullable_action = ON_CONFLICT_ACTION_DEFAULT; -} - /** * Given a field number and a path, add the corresponding field * to the tuple format, allocating intermediate fields if @@ -335,13 +317,11 @@ tuple_format_add_field(struct tuple_format *format, uint32_t fieldno, parent->offset_slot = *current_slot; } } - tuple_field_update_nullability(parent); parent->is_key_part = true; next->is_multikey_part = is_multikey; parent = next; token_count++; } - tuple_field_update_nullability(parent); /* * The path has already been verified by the * key_def_decode_parts function. diff --git a/src/box/vinyl.c b/src/box/vinyl.c index 32301d7ba..fd9b7e6c0 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -651,24 +651,13 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def) index_def->name, space_name(space)); return -1; } - - struct key_def *key_def = index_def->key_def; - - if (key_def->is_nullable && index_def->iid == 0) { + if (index_def->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 < key_def->part_count; i++) { - struct key_part *part = &key_def->parts[i]; + for (uint32_t i = 0; i < index_def->key_def->part_count; i++) { + struct key_part *part = &index_def->key_def->parts[i]; if (part->type <= FIELD_TYPE_ANY || part->type >= FIELD_TYPE_ARRAY) { diag_set(ClientError, ER_MODIFY_INDEX, @@ -678,7 +667,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def) return -1; } } - if (key_def->for_func_index) { + if (index_def->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-extra-fields-nullability.result similarity index 53% rename from test/engine/gh-5027-fields-nullability.result rename to test/engine/gh-5027-extra-fields-nullability.result index 1121727f6..372fe15b8 100644 --- a/test/engine/gh-5027-fields-nullability.result +++ b/test/engine/gh-5027-extra-fields-nullability.result @@ -64,56 +64,3 @@ 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} - | --- - | - [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-extra-fields-nullability.test.lua similarity index 51% rename from test/engine/gh-5027-fields-nullability.test.lua rename to test/engine/gh-5027-extra-fields-nullability.test.lua index 960103d6c..5d72d949a 100644 --- a/test/engine/gh-5027-fields-nullability.test.lua +++ b/test/engine/gh-5027-extra-fields-nullability.test.lua @@ -18,20 +18,3 @@ 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() diff --git a/test/engine/gh-5192-multikey-root-nullability.result b/test/engine/gh-5192-multikey-root-nullability.result new file mode 100644 index 000000000..84a94e2b3 --- /dev/null +++ b/test/engine/gh-5192-multikey-root-nullability.result @@ -0,0 +1,62 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) + | --- + | ... +_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) + | --- + | ... +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) + | --- + | ... +s:replace{1, box.NULL} + | --- + | - [1, null] + | ... +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) + | --- + | ... +s:replace{2, box.NULL} + | --- + | - [2, null] + | ... +_ = s:delete(2) + | --- + | ... +s:truncate() + | --- + | ... +s:drop() + | --- + | ... + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) + | --- + | ... +_ = s:format({{name='id'}, {name='data', type='array'}}) + | --- + | ... +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) + | --- + | ... +s:replace{1, box.NULL} + | --- + | - error: '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-5192-multikey-root-nullability.test.lua b/test/engine/gh-5192-multikey-root-nullability.test.lua new file mode 100644 index 000000000..8a4c45fb8 --- /dev/null +++ b/test/engine/gh-5192-multikey-root-nullability.test.lua @@ -0,0 +1,20 @@ +test_run = require('test_run').new() + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) +_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}}) +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) +s:replace{1, box.NULL} +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) +s:replace{2, box.NULL} +_ = s:delete(2) +s:truncate() +s:drop() + +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')}) +_ = s:format({{name='id'}, {name='data', type='array'}}) +_ = s:create_index('i1', {parts={{1, 'unsigned'}}}) +s:replace{1, box.NULL} +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}}) +s:replace{2, box.NULL} +s:replace{3, {}} +s:drop() \ No newline at end of file diff --git a/test/engine/json.result b/test/engine/json.result index 2175266fc..bc1d7cad5 100644 --- a/test/engine/json.result +++ b/test/engine/json.result @@ -738,12 +738,11 @@ _ = s:create_index('sk', {parts = {{'[2][1].a', 'unsigned'}}}) ... s:insert{1, box.NULL} -- error --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- error: Tuple field [2][1]["a"] required by space format is missing ... s:insert{2, {box.NULL}} -- error --- -- error: 'Tuple field [2][1] type does not match one required by operation: expected - map' +- error: Tuple field [2][1]["a"] required by space format is missing ... s:insert{3} -- error --- @@ -766,12 +765,11 @@ s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}} ... s:insert{7, box.NULL} -- error --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [7, null] ... s:insert{8, {box.NULL}} -- error --- -- error: 'Tuple field [2][1] type does not match one required by operation: expected - map' +- [8, [null]] ... -- Skipping nullable fields is okay though. s:insert{9} -- ok @@ -792,7 +790,9 @@ s:insert{12, {{a = box.NULL}}} -- ok ... s.index.sk:select() --- -- - [9] +- - [7, null] + - [8, [null]] + - [9] - [10, []] - [11, [{'b': 1}]] - [12, [{'a': null}]] diff --git a/test/engine/multikey.result b/test/engine/multikey.result index 968be4cc3..e9005743e 100644 --- a/test/engine/multikey.result +++ b/test/engine/multikey.result @@ -796,7 +796,7 @@ _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}}) ... s:insert{1, box.NULL} -- error --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [1, null] ... s:insert{2, {box.NULL}} -- error --- @@ -816,7 +816,7 @@ s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}} ... s:insert{5, box.NULL} -- still error --- -- error: 'Tuple field 2 type does not match one required by operation: expected array' +- [5, null] ... s:insert{6, {box.NULL}} -- ok --- -- 2.17.1