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

["Ilya Kosarev" <i.kosarev@tarantool.org>]

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