From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 1076E445320 for ; Thu, 30 Jul 2020 17:26:21 +0300 (MSK) Date: Thu, 30 Jul 2020 14:26:20 +0000 From: Nikita Pettik Message-ID: <20200730142620.GA25238@tarantool.org> References: <20200724200633.12122-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200724200633.12122-1-i.kosarev@tarantool.org> Subject: Re: [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: Ilya Kosarev Cc: tarantool-patches@dev.tarantool.org On 24 Jul 23:06, Ilya Kosarev wrote: > 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 > I *strongly dislike* that the patch is in fact should be 10 lines but instead we have 250 diff lines. Please elaborate on that (comments above are mostly about that). > 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) { Again I see this redundant diff. Please drop it, it doesn't fix refactor/fix anything but makes diff bigger and complicates git history. > if (index_def->iid == 0) { > diag_set(ClientError, ER_NULLABLE_PRIMARY, > space_name(space)); > 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); > } > > @@ -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 Please, avoid renaming test.. > index 1121727f6..372fe15b8 100644 > 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 I guess you forgot to fix this comment (and the rest in this test)...