* [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map @ 2020-06-18 17:57 Ilya Kosarev 2020-06-23 10:56 ` Aleksandr Lyapunov 2020-07-03 9:28 ` Nikita Pettik 0 siblings, 2 replies; 5+ messages in thread From: Ilya Kosarev @ 2020-06-18 17:57 UTC (permalink / raw) To: tarantool-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map 2020-06-18 17:57 [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map Ilya Kosarev @ 2020-06-23 10:56 ` Aleksandr Lyapunov 2020-06-29 17:42 ` Ilya Kosarev 2020-07-03 9:28 ` Nikita Pettik 1 sibling, 1 reply; 5+ messages in thread From: Aleksandr Lyapunov @ 2020-06-23 10:56 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map 2020-06-23 10:56 ` Aleksandr Lyapunov @ 2020-06-29 17:42 ` Ilya Kosarev 0 siblings, 0 replies; 5+ messages in thread From: Ilya Kosarev @ 2020-06-29 17:42 UTC (permalink / raw) To: Aleksandr Lyapunov; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map 2020-06-18 17:57 [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map Ilya Kosarev 2020-06-23 10:56 ` Aleksandr Lyapunov @ 2020-07-03 9:28 ` Nikita Pettik 2020-07-12 13:43 ` Ilya Kosarev 1 sibling, 1 reply; 5+ messages in thread From: Nikita Pettik @ 2020-07-03 9:28 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map 2020-07-03 9:28 ` Nikita Pettik @ 2020-07-12 13:43 ` Ilya Kosarev 0 siblings, 0 replies; 5+ messages in thread From: Ilya Kosarev @ 2020-07-12 13:43 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-12 13:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-18 17:57 [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map Ilya Kosarev 2020-06-23 10:56 ` Aleksandr Lyapunov 2020-06-29 17:42 ` Ilya Kosarev 2020-07-03 9:28 ` Nikita Pettik 2020-07-12 13:43 ` Ilya Kosarev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox