From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 48A48445320 for ; Tue, 14 Jul 2020 13:37:17 +0300 (MSK) Date: Tue, 14 Jul 2020 10:37:16 +0000 From: Nikita Pettik Message-ID: <20200714103715.GA19119@tarantool.org> References: <20200713123633.13961-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200713123633.13961-1-i.kosarev@tarantool.org> 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 Cc: tarantool-patches@dev.tarantool.org 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 >