Re: [Tarantool-patches] [PATCH] tuple: drop extra restrictions for multikey index

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5889 bytes --]


Hi!
 
Thanks for your review.
 
See my answers below.
 
I will send v2 of the patch but i think 2 questions above should be clarified.
  
>Четверг, 30 июля 2020, 17:26 +03:00 от Nikita Pettik <korablev@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).
Well, this patch reverts the majority of changes from
4cf94ef8cb90b84ea71f313cff3e016f85894fd5 (tuple: make fields nullable
by default except array/map). That is where the size comes from.
>
>> 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.
The problem is that this is the reversion of that diff. Well, I guess
as far as that patch was pushed to master we can leave that change
there and revert only the meaningful part?
>
>> 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..
I guess we can drop this change, however i thought it is a good idea
to make test name more clear, as you proposed:
>>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.
Is it really that bad to rename?
>
>> 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)…
Right, this comments have to be fixed.
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 7991 bytes --]