

<HTML><BODY><div>Hi!<br><br>Sent v3 considering your comments for Alexander L. to review it.<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 1 октября 2020, 14:47 +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_16015528520353722863_BODY">LGTM.<br><br>I see a bit different way to do the same, but I don't know whether it is<br>better. So I would say that both ways are okay for me, but it would be<br>good if someone other would look into this discussion too. I hope<br>Alexander L. will do.<br><br>See also the comment regarding the dubious test case.<br><br>WBR, Alexander Turenko.<br><br>> + /**<br>> + * First part type not supported for comparison.<br>> + * Valid if key_def does not support comparison<br>> + * (key_def->tuple_compare* == NULL), undefined othewise.<br><br>I would set it to field_type_MAX in the case, but maybe it is just my<br>fear of undefined values.<br><br>> + */<br>> + enum field_type unsupported_type;<br><br>I see the alternative (but I don't know whether it is better). We can<br>wrap the loop into a function and use it directly on the error path<br>(when key_def->tuple_compare or key_def->tuple_compare_with_key is NULL)<br>to obtain an error message. A performance of the error path is not<br>important I guess. This way we'll not need the extra field in <struct<br>key_def>.</div></div></div></div></blockquote></div><div>Right, i think it is a good idea. Didn’t think that we can just do it on the error path.</div><div>I will implement it this way, but without extra diag_set().</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>Unlikely we'll create a lot of key_defs, so the question is purely how<br>the code would look better. A field just to pass it to an error message<br>is a bit artificial thing, IMHO.<br><br>Let the check function (key_def_has_comparator() or so) to set the<br>diagnostics and so the checking code and the construction of an error<br>message will be in one place. The usage would be like so:<br><br>In key_def_set_compare_func():<br><br> | if (! key_def_has_comparator(def)) {<br> | def->tuple_compare = NULL;<br> | def->tuple_compare_with_key = NULL;<br> | }<br><br>In lbox_key_def_compare():<br><br> | if (key_def->tuple_compare == NULL) {<br> | /* Just to set an error to the diagnostics area. */<br> | int rc = key_def_has_comparator(key_def);<br> | (void) rc;<br> | assert(rc != 0);<br> | return luaT_error(L);<br> | }<br><br>But I'll repeat: I don't know whether it worth to do. Feel free to<br>ignore.<br><br>> + if (key_def->tuple_compare == NULL) {<br>> + diag_set(IllegalParams, "Unsupported field type: %s",<br>> + field_type_strs[key_def->unsupported_type]);<br>> + return luaT_error(L);<br>> + }<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 +343,12 @@ lbox_key_def_compare_with_key(struct lua_State *L)<br>> "compare_with_key(tuple, key)");<br>> }<br>><br>> + if (key_def->tuple_compare_with_key == NULL) {<br>> + diag_set(IllegalParams, "Unsupported field type: %s",<br>> + field_type_strs[key_def->unsupported_type]);<br>> + return luaT_error(L);<br>> + }<br>> +<br>> struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2);<br>> if (tuple == NULL)<br>> return luaT_error(L);<br><br>(Left here for context.)<br><br>> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc<br>> index d059c709f..762bc8019 100644<br>> --- a/src/box/tuple_compare.cc<br>> +++ b/src/box/tuple_compare.cc<br>> @@ -2081,5 +2081,16 @@ key_def_set_compare_func(struct key_def *def)<br>> key_def_set_compare_func_json<false, false>(def);<br>> }<br>> }<br>> + for (uint32_t i = 0; i < def->part_count; ++i) {<br>> + if (def->parts[i].type == FIELD_TYPE_ANY ||<br>> + def->parts[i].type == FIELD_TYPE_ARRAY ||<br>> + def->parts[i].type == FIELD_TYPE_MAP) {<br>> + /* Tuple comparators don't support these types. */<br>> + def->tuple_compare = NULL;<br>> + def->tuple_compare_with_key = NULL;<br>> + def->unsupported_type = def->parts[i].type;<br>> + break;<br>> + }<br>> + }<br>> key_def_set_hint_func(def);<br>> }<br><br>(Left here for context.)<br><br>> @@ -488,6 +525,35 @@ test:test('merge()', function(test)<br>> 'case 3: verify with :totable()')<br>> test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),<br>> {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')<br>> +<br>> + local parts_unsigned = {<br>> + {type = 'unsigned', fieldno = 1, is_nullable = false},<br>> + }<br>> + local key_def_unsigned = key_def_lib.new(parts_unsigned)<br>> + local key_def_string = key_def_lib.new({<br>> + {type = 'string', fieldno = 1},<br>> + })<br>> + local key_def_array = key_def_lib.new({<br>> + {type = 'array', fieldno = 1},<br>> + {type = 'unsigned', fieldno = 2},<br>> + })<br>> + local key_def_map = key_def_lib.new({<br>> + {type = 'map', fieldno = 3, is_nullable = true},<br>> + {type = 'scalar', fieldno = 2},<br>> + })<br>> +<br>> + local key_def_unsigned_string = key_def_unsigned:merge(key_def_string)<br>> + test:is_deeply(key_def_unsigned_string:totable(), parts_unsigned,<br>> + 'in case of conflict we just get the field from the first key_def')<br><br>If you add a test case and not sure whether corrsponding behaviour is<br>right, there are two options. Either clarify that you just hold current<br>behaviour and presence of this test case does not mean that the<br>behaviour should remain the same in future (but what is purpose of the<br>case so?). Or don't add it.</div></div></div></div></blockquote></div><div>Right! I will clarify the fact this behavior is not intended to be saved.</div><div>Though i think it is useful to make it transparent how it works now.</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>Just to avoid any possible confusion like 'we test the behaviour, so it<br>seems there is some commitment that we'll keep it'.<br><br>I shared my doubts about this behaviour in [1].<br><br>[1]: <a href="https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019807.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019807.html</a></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>