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).

> 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..