From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 6B37A445320 for ; Mon, 13 Jul 2020 14:19:09 +0300 (MSK) References: <20200712134316.31126-1-i.kosarev@tarantool.org> From: Aleksandr Lyapunov Message-ID: Date: Mon, 13 Jul 2020 14:19:07 +0300 MIME-Version: 1.0 In-Reply-To: <20200712134316.31126-1-i.kosarev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [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: Ilya Kosarev , tarantool-patches@dev.tarantool.org 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()