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

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

Hello, thanks for the patch!
I don't like it nevertheless.
Btw I don't like Vladimir's patch about required map/array either.
It seems that there are to many ways to declare field as nullable,
at least by setting nullable_action and removing corresponding bit
from required_fileds mask.
There must one more way since tarantool from master allows
to insert tuples like that:
s:create_index('pk', {parts={{1, uint}}})
s:create_index('test', {parts={{4, uint}}})
s:replace{1, box.NULL, nil, 1}

That means that the case above 2nd and 3rd fields are nullable
by default.

It all looks like a mess, I fear that you have to dig more and create
the one and only way to set fields as required or not.

Please see my comment below.

On 6/18/20 8:57 PM, 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
> ---
> 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;
>   		}
> +		if ((field->type == FIELD_TYPE_ARRAY ||
> +		    field->type == FIELD_TYPE_MAP) &&
> +		    tuple_field_is_nullable(field))
> +			field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
That doesn't look like a proper place to edit field.
If array and map fields are not allowed to be nullable then two lines
above the filed was in inconsistent state - with array/map type and 
nullable.
That's bad. We should set type and nullable_action at least in adjacent 
lines.
>   		/*
>   		 * 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
> +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}
> +---
> +- [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}
> +---
> +- 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