From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 55AFF42EF5C for ; Tue, 23 Jun 2020 13:56:50 +0300 (MSK) References: <20200618175720.27089-1-i.kosarev@tarantool.org> From: Aleksandr Lyapunov Message-ID: Date: Tue, 23 Jun 2020 13:56:49 +0300 MIME-Version: 1.0 In-Reply-To: <20200618175720.27089-1-i.kosarev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , tarantool-patches@dev.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