From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 C048B4429E1 for ; Fri, 3 Jul 2020 12:28:52 +0300 (MSK) Date: Fri, 3 Jul 2020 09:28:51 +0000 From: Nikita Pettik Message-ID: <20200703092851.GA11096@tarantool.org> References: <20200618175720.27089-1-i.kosarev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200618175720.27089-1-i.kosarev@tarantool.org> 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 Cc: tarantool-patches@dev.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.