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

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

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