<HTML><BODY><div><div>Hi,<br><br>Thanks for your review!</div><div> </div><div>Sent v2 of the patch considering your comments.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница, 25 сентября 2020, 21:34 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_16010588800932073187_BODY">I have no objections in general, but there are doubts around several<br>places. Please, look below.<br><br>WBR, Alexander Turenko.<br><br>> +static bool<br>> +key_def_comparable(struct key_def *key_def)<br><br>What make me doubt: key_def is not comparable per se, it may or may not<br>be used for comparison of tuples and a tuple with a key.<br>'key_def_has_comparator' or 'key_def_can_compare' (however not key_def<br>itself perform comparisons, hmm), maybe, don't know.</div></div></div></div></blockquote></div><div>Well, yes. I meant key_def as a module affiliation here, not like an object.</div><div>Good thing is that we don’t really need separate function with the new approach!</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>> +{<br>> + for (uint32_t i = 0; i < key_def->part_count; ++i) {<br>> + if (key_def->parts[i].type == FIELD_TYPE_ANY ||<br>> + key_def->parts[i].type == FIELD_TYPE_ARRAY ||<br>> + key_def->parts[i].type == FIELD_TYPE_MAP) {<br>> + /* Tuple comparators don't support these types. */<br>> + diag_set(IllegalParams, "Unsupported field type: %s",<br>> + field_type_strs[key_def->parts[i].type]);<br>> + return false;<br>> + }<br>> + }<br>> + return true;<br>> +}<br>> +<br><br>Ilya gives the idea: perform this check on key_def creation and store a<br>flag inside key_def. Check against the flag in lbox_key_def_compare()<br>and lbox_key_def_compare_with_key().<br><br>This looks as the right way to solve this kind of problems: comparisons<br>are more hot functions than key_def creation.<br><br>We can sink it down to key_def_set_compare_func() and set NULL to<br>key_def->{tuple_compare,tuple_compare_with_key}. Than check it in<br>lbox_key_def_compare*() and add asserts to tuple_compare*(). No new<br>fields will be required so.</div></div></div></div></blockquote></div><div>Yes! This seems like the right choice. Though we still need a field here:</div><div>somewhere to store the unsupported type to show it to the user any time</div><div>he tries to compare. Implemented in v2 of the patch.</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>This part surely should look someone, who is more near to comparators<br>than me.<br><br>> /**<br>> * Free a key_def from a Lua code.<br>> */<br>> @@ -316,6 +320,9 @@ lbox_key_def_compare(struct lua_State *L)<br>> "compare(tuple_a, tuple_b)");<br>> }<br>><br>> + if (!key_def_comparable(key_def))<br>> + return luaT_error(L);<br>> +<br>> struct tuple *tuple_a, *tuple_b;<br>> if ((tuple_a = luaT_key_def_check_tuple(L, key_def, 2)) == NULL)<br>> return luaT_error(L);<br>> @@ -349,6 +356,9 @@ lbox_key_def_compare_with_key(struct lua_State *L)<br>> "compare_with_key(tuple, key)");<br>> }<br>><br>> + if (!key_def_comparable(key_def))<br>> + return luaT_error(L);<br>> +<br>> struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);<br>> if (tuple == NULL)<br>> return luaT_error(L);<br>> diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua<br>> index 3a4aad68721..8fcdf7070bf 100755<br><br>How about lbox_key_def_merge() and underlying functions? I'm not sure<br>they will work correct. At least I tried this on the branch:<br><br> | tarantool> key_def = require('key_def')<br> | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'array'}})<br> | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'map'}})<br> | tarantool> kd1:merge(kd2)<br> | ---<br> | - - type: array<br> | is_nullable: false<br> | fieldno: 1<br> | ...<br><br>It does not look correct.</div></div></div></div></blockquote></div><div>This turns out to be the correct behavior:</div><div> </div><div><div style="margin-left:40px">tarantool> kd1 = key_def.new({{fieldno = 1, type = 'unsigned'}})<br>---<br>...</div><div style="margin-left:40px">tarantool> kd2 = key_def.new({{fieldno = 1, type = 'string'}})<br>---<br>...</div><div style="margin-left:40px">tarantool> kd1:merge(kd2)<br>---<br>- - type: unsigned<br> is_nullable: false<br> fieldno: 1<br>…</div><div> </div><div>I decided these functions don’t really interfere and didn’t manage to test.</div><div>Although this is kinda true for now, that wasn’t a correct decision, as far as</div><div>behavior might be changed. Adding the tests in the v2 of the patch.</div></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>Everything looks good with lbox_key_def_to_table(), but I would add a<br>test anyway.</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>