<HTML><BODY><div><div>Hi!<br><br>Thanks for the review.</div><div> </div><div>4 answers below.<br> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 23 июня 2020, 13:56 +03:00 от Aleksandr Lyapunov <alyapunov@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15929098100041291864_BODY">Hello, thanks for the patch!<br>I don't like it nevertheless.<br>Btw I don't like Vladimir's patch about required map/array either.<br>It seems that there are to many ways to declare field as nullable,<br>at least by setting nullable_action and removing corresponding bit<br>from required_fileds mask.</div></div></div></div></blockquote></div><div>1. Although there is more than one way, i think that setting<br>nullable_action in <span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">tuple_field_new</span> is the default way to do this.<br><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">required_fields</span> depend on it:</div><div><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;"> if (json_token_is_leaf(&field->token) &&<br> !tuple_field_is_nullable(field))<br> bit_set(required_fields, field->id);</span></div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>There must one more way since tarantool from master allows<br>to insert tuples like that:<br>s:create_index('pk', {parts={{1, uint}}})<br>s:create_index('test', {parts={{4, uint}}})<br>s:replace{1, box.NULL, nil, 1}<br><br>That means that the case above 2nd and 3rd fields are nullable<br>by default.</div></div></div></div></blockquote></div><div>2. These fields are not really nullable. The reason it is possible is the<br>way of format verification. Here are some more examples:</div><div><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>s:insert{1, nil, nil, nil, 5} -- ok<br>s:insert{1, nil, nil, nil} -- error</span></div><div> </div><div>Even in case we explicitly specify the field as non-nullable without any<br>other details it still goes fine (which seems to be even more incorrect):</div><div><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">s:format({{name='field1', is_nullable=false}})<br>i1 = s:create_index('i1', {parts={{2, 'unsigned'}}})<br>i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>s:insert{nil, 2, nil, nil, 5} -- ok</span></div><div> </div><div><span style="font-family:Arial;font-size:15px;line-height:20px;">This way, with type specified, it finally gives us an error:</span></div><div><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">s:format({{name='field1', 'unsigned', is_nullable=false}})<br>i1 = s:create_index('i1', {parts={{2, 'unsigned'}}})<br>i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>s:insert{nil, 2, nil, nil, 5} -- error</span></div><div> </div><div><span style="font-family:Arial;font-size:15px;line-height:20px;">It also works fine (giving an error) with index parts:</span></div><div><span style="font-family:"Andale Mono","Lucida Console",Monaco,monospace;font-size:14px;line-height:20px;">i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>i3 = s:create_index('i3', {parts={{3, is_nullable=false}}})<br>s:insert{1, nil, nil, nil, 5} -- error</span></div><div> </div><div><span style="font-family:Arial;font-size:15px;line-height:20px;">I think that required fields verification on insert needs to be fixed.<br>But it seems to be the issue for another patch.</span></div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>It all looks like a mess, I fear that you have to dig more and create<br>the one and only way to set fields as required or not.</div></div></div></div></blockquote></div><div>3. I tried to think of a nicer way to create tuple format, but it seems to</div><div>be unsimplifiable.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>Please see my comment below.<br><br>On 6/18/20 8:57 PM, Ilya Kosarev wrote:<br>> Since e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't<br>> allow null where array/map is expected) tuple fields are nullable by<br>> default. It seems strange at least in case we have implicit fields in<br>> front of explicit nullable field. Now fields are nullable by default<br>> except arrays & maps due to reasons specified in mentioned commit.<br>><br>> Closes #5027<br>> ---<br>> Branch: <a href="https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability" target="_blank">https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability</a><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/5027" target="_blank">https://github.com/tarantool/tarantool/issues/5027</a><br>><br>> @ChangeLog:<br>> * Fix confusing implicit requirements for tuple fields.<br>><br>> src/box/tuple_format.c | 6 +++++-<br>> test/engine/insert.result | 33 +++++++++++++++++++++++++++++++++<br>> test/engine/insert.test.lua | 12 ++++++++++++<br>> 3 files changed, 50 insertions(+), 1 deletion(-)<br>><br>> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c<br>> index 68ec2a7499..4b8f32149e 100644<br>> --- a/src/box/tuple_format.c<br>> +++ b/src/box/tuple_format.c<br>> @@ -152,7 +152,7 @@ tuple_field_new(void)<br>> field->type = FIELD_TYPE_ANY;<br>> field->offset_slot = TUPLE_OFFSET_SLOT_NIL;<br>> field->coll_id = COLL_NONE;<br>> - field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;<br>> + field->nullable_action = ON_CONFLICT_ACTION_NONE;<br>> field->multikey_required_fields = NULL;<br>> return field;<br>> }<br>> @@ -528,6 +528,10 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,<br>> multikey_required_fields;<br>> required_fields = multikey_required_fields;<br>> }<br>> + if ((field->type == FIELD_TYPE_ARRAY ||<br>> + field->type == FIELD_TYPE_MAP) &&<br>> + tuple_field_is_nullable(field))<br>> + field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;<br>That doesn't look like a proper place to edit field.<br>If array and map fields are not allowed to be nullable then two lines<br>above the filed was in inconsistent state - with array/map type and<br>nullable.<br>That's bad. We should set type and nullable_action at least in adjacent<br>lines.</div></div></div></div></blockquote></div><div>4. Tuple format is mostly created in tuple_format_new which in turn<br>uses tuple_format_alloc and tuple_format_create. Depending on the field<br>features it's type might be specified in different places, for example,<br>deep in tuple_format_add_field. This means that if we want to set<br>nullability right after we set the type we will have to put this logic<br>into an amount of places, which doesn't seem to be nice. That's why I<br>chose to put this logic just before setting required_fields. I think it<br>might still be consistent as far as we are still inside of format<br>creation process and we depend on previously collected type info. </div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>> /*<br>> * Mark all leaf non-nullable fields as required<br>> * by setting the corresponding bit in the bitmap<br>> diff --git a/test/engine/insert.result b/test/engine/insert.result<br>> index 13ffe8ceb1..37a4e8cc68 100644<br>> --- a/test/engine/insert.result<br>> +++ b/test/engine/insert.result<br>> @@ -988,3 +988,36 @@ s:select()<br>> s:drop()<br>> ---<br>> ...<br>> +-- gh-5027: Do not require extra fields<br>> +s = box.schema.space.create('test')<br>> +---<br>> +...<br>> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>> +---<br>> +...<br>> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>> +---<br>> +...<br>> +s:insert{1, 2, 3}<br>> +---<br>> +- [1, 2, 3]<br>> +...<br>> +s:drop()<br>> +---<br>> +...<br>> +s = box.schema.space.create('test')<br>> +---<br>> +...<br>> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>> +---<br>> +...<br>> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})<br>> +---<br>> +...<br>> +s:insert{1, 2, 3}<br>> +---<br>> +- error: Tuple field 5 required by space format is missing<br>> +...<br>> +s:drop()<br>> +---<br>> +...<br>> diff --git a/test/engine/insert.test.lua b/test/engine/insert.test.lua<br>> index 2ffc8cd0a0..112ca62c53 100644<br>> --- a/test/engine/insert.test.lua<br>> +++ b/test/engine/insert.test.lua<br>> @@ -210,3 +210,15 @@ ddd:delete(ffi.cast('double', 123))<br>> s:select()<br>><br>> s:drop()<br>> +<br>> +-- gh-5027: Do not require extra fields<br>> +s = box.schema.space.create('test')<br>> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})<br>> +s:insert{1, 2, 3}<br>> +s:drop()<br>> +s = box.schema.space.create('test')<br>> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})<br>> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})<br>> +s:insert{1, 2, 3}<br>> +s:drop()<br>> \ No newline at end of file</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></BODY></HTML>