<HTML><BODY><div><div>Hi!</div><div> </div><div>Thanks for your review.</div><div> </div><div>See my answers below.</div><div> </div><div>I will send v2 of the patch but i think 2 questions above should be clarified.<br> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 30 июля 2020, 17:26 +03:00 от Nikita Pettik <korablev@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15961191811024768262_BODY">On 24 Jul 23:06, Ilya Kosarev wrote:<br>> Multikey index did not work properly with nullable root field in<br>> tuple_raw_multikey_count(). Now it is fixed and corresponding<br>> restrictions are dropped. This also means that we can drop implicit<br>> nullability update for array/map fields and make all fields nullable<br>> by default, as it was until e1d3fe8ab8eed65394ad17409401a93b6fcdc435<br>> (tuple format: don't allow null where array/map is expected), as far as<br>> default non-nullability itself doesn't solve any real problems while<br>> providing confusing behavior (gh-5027).<br>><br>> Follow-up #5027<br>> Closes #5192<br>> ---<br>> Branch: <a href="https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions" target="_blank">https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5192-fix-multikey-index-restrictions</a><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/5192" target="_blank">https://github.com/tarantool/tarantool/issues/5192</a><br>><br><br>I *strongly dislike* that the patch is in fact should be 10 lines<br>but instead we have 250 diff lines. Please elaborate on that<br>(comments above are mostly about that).</div></div></div></div></blockquote></div><div>Well, this patch reverts the majority of changes from<br>4cf94ef8cb90b84ea71f313cff3e016f85894fd5 (tuple: make fields nullable<br>by default except array/map). That is where the size comes from.</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>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c<br>> index 8452ab430..875592026 100644<br>> --- a/src/box/memtx_space.c<br>> +++ b/src/box/memtx_space.c<br>> @@ -589,9 +589,7 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid)<br>> static int<br>> memtx_space_check_index_def(struct space *space, struct index_def *index_def)<br>> {<br>> - struct key_def *key_def = index_def->key_def;<br>> -<br>> - if (key_def->is_nullable) {<br>> + if (index_def->key_def->is_nullable) {<br><br>Again I see this redundant diff. Please drop it, it doesn't fix<br>refactor/fix anything but makes diff bigger and complicates git<br>history.</div></div></div></div></blockquote></div><div>The problem is that this is the reversion of that diff. Well, I guess<br>as far as that patch was pushed to master we can leave that change<br>there and revert only the meaningful part?</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>> if (index_def->iid == 0) {<br>> diag_set(ClientError, ER_NULLABLE_PRIMARY,<br>> space_name(space));<br>> diff --git a/src/box/tuple.c b/src/box/tuple.c<br>> index 9f0f24c64..94a3a96ba 100644<br>> --- a/src/box/tuple.c<br>> +++ b/src/box/tuple.c<br>> @@ -554,7 +554,10 @@ tuple_raw_multikey_count(struct tuple_format *format, const char *data,<br>> NULL, MULTIKEY_NONE);<br>> if (array_raw == NULL)<br>> return 0;<br>> - assert(mp_typeof(*array_raw) == MP_ARRAY);<br>> + enum mp_type type = mp_typeof(*array_raw);<br>> + if (type == MP_NIL)<br>> + return 0;<br>> + assert(type == MP_ARRAY);<br>> return mp_decode_array(&array_raw);<br>> }<br>><br>> @@ -335,13 +317,11 @@ tuple_format_add_field(struct tuple_format *format, uint32_t fieldno,<br>> parent->offset_slot = *current_slot;<br>> }<br>> }<br>> - tuple_field_update_nullability(parent);<br>> parent->is_key_part = true;<br>> next->is_multikey_part = is_multikey;<br>> parent = next;<br>> token_count++;<br>> }<br>> - tuple_field_update_nullability(parent);<br>> /*<br>> * The path has already been verified by the<br>> * key_def_decode_parts function.<br>> diff --git a/src/box/vinyl.c b/src/box/vinyl.c<br>> index 32301d7ba..fd9b7e6c0 100644<br>> --- a/src/box/vinyl.c<br>> +++ b/src/box/vinyl.c<br>> @@ -651,24 +651,13 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)<br>> index_def->name, space_name(space));<br>> return -1;<br>> }<br>> -<br>> - struct key_def *key_def = index_def->key_def;<br>> -<br>> - if (key_def->is_nullable && index_def->iid == 0) {<br>> + if (index_def->key_def->is_nullable && index_def->iid == 0) {<br>> diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name(space));<br>> return -1;<br>> }<br>> - if (key_def->is_multikey &&<br>> - key_def->multikey_fieldno < space->def->field_count &&<br>> - space->def->fields[key_def->multikey_fieldno].is_nullable) {<br>> - diag_set(ClientError, ER_UNSUPPORTED,<br>> - "multikey index",<br>> - "nullable root field");<br>> - return -1;<br>> - }<br>> /* Check that there are no ANY, ARRAY, MAP parts */<br>> - for (uint32_t i = 0; i < key_def->part_count; i++) {<br>> - struct key_part *part = &key_def->parts[i];<br>> + for (uint32_t i = 0; i < index_def->key_def->part_count; i++) {<br>> + struct key_part *part = &index_def->key_def->parts[i];<br>> if (part->type <= FIELD_TYPE_ANY ||<br>> part->type >= FIELD_TYPE_ARRAY) {<br>> diag_set(ClientError, ER_MODIFY_INDEX,<br>> @@ -678,7 +667,7 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)<br>> return -1;<br>> }<br>> }<br>> - if (key_def->for_func_index) {<br>> + if (index_def->key_def->for_func_index) {<br>> diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",<br>> "functional index");<br>> return -1;<br>> diff --git a/test/engine/gh-5027-fields-nullability.result b/test/engine/gh-5027-extra-fields-nullability.result<br>> similarity index 53%<br>> rename from test/engine/gh-5027-fields-nullability.result<br>> rename to test/engine/gh-5027-extra-fields-nullability.result<br><br>Please, avoid renaming test..</div></div></div></div></blockquote></div><div>I guess we can drop this change, however i thought it is a good idea</div><div>to make test name more clear, as you proposed:</div><div><pre>>>Nit: mb gh-5027-extra-fields-nullability or gh-5027-compound-field-type-nullability?

>>Then others would understand what this test is about with ease.</pre><div>Is it really that bad to rename?</div></div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div class="js-helper js-readmsg-msg"><br>> index 1121727f6..372fe15b8 100644<br>> diff --git a/test/engine/multikey.result b/test/engine/multikey.result<br>> index 968be4cc3..e9005743e 100644<br>> --- a/test/engine/multikey.result<br>> +++ b/test/engine/multikey.result<br>> @@ -796,7 +796,7 @@ _ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})<br>> ...<br>> s:insert{1, box.NULL} -- error<br><br>I guess you forgot to fix this comment (and the rest in this test)…</div></blockquote></div><div>Right, this comments have to be fixed.</div><div><div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></BODY></HTML>