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