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

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

[Ilya Kosarev]

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;
 		/*
 		 * 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
-- 
2.17.1

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

[Aleksandr Lyapunov]

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

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

[Ilya Kosarev]

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


Hi!

Thanks for the review.
 
4 answers below.
  
>Вторник, 23 июня 2020, 13:56 +03:00 от 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.
1. Although there is more than one way, i think that setting
nullable_action in  tuple_field_new is the default way to do this.
required_fields depend on it:
    if (json_token_is_leaf(&field->token) &&
        !tuple_field_is_nullable(field))
      bit_set(required_fields, field->id);
>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.
2. These fields are not really nullable. The reason it is possible is the
way of format verification. Here are some more examples:
i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
s:insert{1, nil, nil, nil, 5} -- ok
s:insert{1, nil, nil, nil} -- error
 
Even in case we explicitly specify the field as non-nullable without any
other details it still goes fine (which seems to be even more incorrect):
s:format({{name='field1', is_nullable=false}})
i1 = s:create_index('i1', {parts={{2, 'unsigned'}}})
i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
s:insert{nil, 2, nil, nil, 5} -- ok
 
This way, with type specified, it finally gives us an error:
s:format({{name='field1', 'unsigned', is_nullable=false}})
i1 = s:create_index('i1', {parts={{2, 'unsigned'}}})
i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
s:insert{nil, 2, nil, nil, 5} -- error
 
It also works fine (giving an error) with index parts:
i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
i3 = s:create_index('i3', {parts={{3, is_nullable=false}}})
s:insert{1, nil, nil, nil, 5} -- error
 
I think that required fields verification on insert needs to be fixed.
But it seems to be the issue for another patch.
>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.
3. I tried to think of a nicer way to create tuple format, but it seems to
be unsimplifiable.
>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.
4. Tuple format is mostly created in tuple_format_new which in turn
uses tuple_format_alloc and tuple_format_create. Depending on the field
features it's type might be specified in different places, for example,
deep in tuple_format_add_field. This means that if we want to set
nullability right after we set the type we will have to put this logic
into an amount of places, which doesn't seem to be nice. That's why I
chose to put this logic just before setting required_fields. I think it
might still be consistent as far as we are still inside of format
creation process and we depend on previously collected type info. 
>> /*
>> * 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 
 
 
--
Ilya Kosarev
 

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

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

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

[Ilya Kosarev]

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


Hi!
 
Thanks for the review.
 
Sent v2 of the patch considering your comments.
  
>Пятница, 3 июля 2020, 12:28 +03:00 от Nikita Pettik <korablev@tarantool.org>:
> 
>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.
>  
 
 
--
Ilya Kosarev
 

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