From: Alexander Turenko <alexander.turenko@tarantool.org> To: Ilya Kosarev <i.kosarev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] key_def: support composite types extraction Date: Fri, 25 Sep 2020 21:34:50 +0300 [thread overview] Message-ID: <20200925183450.atsuxr4ine7c2dv7@tkn_work_nb> (raw) In-Reply-To: <20200921121100.10052-1-i.kosarev@tarantool.org> I have no objections in general, but there are doubts around several places. Please, look below. WBR, Alexander Turenko. > +static bool > +key_def_comparable(struct key_def *key_def) What make me doubt: key_def is not comparable per se, it may or may not be used for comparison of tuples and a tuple with a key. 'key_def_has_comparator' or 'key_def_can_compare' (however not key_def itself perform comparisons, hmm), maybe, don't know. > +{ > + for (uint32_t i = 0; i < key_def->part_count; ++i) { > + if (key_def->parts[i].type == FIELD_TYPE_ANY || > + key_def->parts[i].type == FIELD_TYPE_ARRAY || > + key_def->parts[i].type == FIELD_TYPE_MAP) { > + /* Tuple comparators don't support these types. */ > + diag_set(IllegalParams, "Unsupported field type: %s", > + field_type_strs[key_def->parts[i].type]); > + return false; > + } > + } > + return true; > +} > + Ilya gives the idea: perform this check on key_def creation and store a flag inside key_def. Check against the flag in lbox_key_def_compare() and lbox_key_def_compare_with_key(). This looks as the right way to solve this kind of problems: comparisons are more hot functions than key_def creation. We can sink it down to key_def_set_compare_func() and set NULL to key_def->{tuple_compare,tuple_compare_with_key}. Than check it in lbox_key_def_compare*() and add asserts to tuple_compare*(). No new fields will be required so. This part surely should look someone, who is more near to comparators than me. > /** > * Free a key_def from a Lua code. > */ > @@ -316,6 +320,9 @@ lbox_key_def_compare(struct lua_State *L) > "compare(tuple_a, tuple_b)"); > } > > + if (!key_def_comparable(key_def)) > + return luaT_error(L); > + > struct tuple *tuple_a, *tuple_b; > if ((tuple_a = luaT_key_def_check_tuple(L, key_def, 2)) == NULL) > return luaT_error(L); > @@ -349,6 +356,9 @@ lbox_key_def_compare_with_key(struct lua_State *L) > "compare_with_key(tuple, key)"); > } > > + if (!key_def_comparable(key_def)) > + return luaT_error(L); > + > struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2); > if (tuple == NULL) > return luaT_error(L); > diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua > index 3a4aad68721..8fcdf7070bf 100755 How about lbox_key_def_merge() and underlying functions? I'm not sure they will work correct. At least I tried this on the branch: | tarantool> key_def = require('key_def') | tarantool> kd1 = key_def.new({{fieldno = 1, type = 'array'}}) | tarantool> kd2 = key_def.new({{fieldno = 1, type = 'map'}}) | tarantool> kd1:merge(kd2) | --- | - - type: array | is_nullable: false | fieldno: 1 | ... It does not look correct. Everything looks good with lbox_key_def_to_table(), but I would add a test anyway.
next prev parent reply other threads:[~2020-09-25 18:34 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-21 12:11 Ilya Kosarev 2020-09-25 18:34 ` Alexander Turenko [this message] 2020-09-26 21:53 ` Ilya Kosarev 2020-10-01 11:38 ` Alexander Turenko 2020-10-01 15:26 ` Ilya Kosarev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200925183450.atsuxr4ine7c2dv7@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=i.kosarev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] key_def: support composite types extraction' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox