[Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map

[Nikita Pettik]

On 18 Jun 20:57, Ilya Kosarev wrote:
> Since e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't
> allow null where array/map is expected) tuple fields are nullable by
> default. It seems strange at least in case we have implicit fields in
> front of explicit nullable field. Now fields are nullable by default
> except arrays & maps due to reasons specified in mentioned commit.
> 
> Closes #5027
> ---

I believe your patch indeed fixes the problem. But I have
no idea how it is connected with map/array fields, since
in the test case field type is unsigned. Could you please
provide more info concerning fix&problem topic?

> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability
> Issue: https://github.com/tarantool/tarantool/issues/5027
> 
> @ChangeLog:
>  * Fix confusing implicit requirements for tuple fields.
> 
>  src/box/tuple_format.c      |  6 +++++-
>  test/engine/insert.result   | 33 +++++++++++++++++++++++++++++++++
>  test/engine/insert.test.lua | 12 ++++++++++++
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 68ec2a7499..4b8f32149e 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;
>  }
> @@ -528,6 +528,10 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>  				multikey_required_fields;
>  			required_fields = multikey_required_fields;
>  		}

This snippet definitely lacks good comment...

> +		if ((field->type == FIELD_TYPE_ARRAY ||
> +		    field->type == FIELD_TYPE_MAP) &&
> +		    tuple_field_is_nullable(field))
> +			field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
>  		/*
>  		 * Mark all leaf non-nullable fields as required
>  		 * by setting the corresponding bit in the bitmap
> diff --git a/test/engine/insert.result b/test/engine/insert.result
> index 13ffe8ceb1..37a4e8cc68 100644
> --- a/test/engine/insert.result
> +++ b/test/engine/insert.result
> @@ -988,3 +988,36 @@ s:select()
>  s:drop()
>  ---
>  ...
> +-- gh-5027: Do not require extra fields

AFAIR we have agreement that regression tests are extracted
in a separate file.

> +s = box.schema.space.create('test')
> +---
> +...
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +---
> +...
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
> +---
> +...
> +s:insert{1, 2, 3}

Why not use test case right from the issue?

> +s:insert{1, 2, 3}
> +---
> +- error: Tuple field 5 required by space format is missing
> +...
> +s:drop()
> +---
> +...
> diff --git a/test/engine/insert.test.lua b/test/engine/insert.test.lua
> index 2ffc8cd0a0..112ca62c53 100644
> --- a/test/engine/insert.test.lua
> +++ b/test/engine/insert.test.lua
> @@ -210,3 +210,15 @@ ddd:delete(ffi.cast('double', 123))
>  s:select()
>  
>  s:drop()
> +
> +-- gh-5027: Do not require extra fields
> +s = box.schema.space.create('test')
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
> +s:insert{1, 2, 3}
> +s:drop()
> +s = box.schema.space.create('test')
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})
> +s:insert{1, 2, 3}
> +s:drop()
> \ No newline at end of file

Let's put newline, this message looks annoying.