From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH v2 0/5] box: introduce multikey indexes in memtx References: <20190328102121.3arq2o4iflksxsio@esperanza> From: Kirill Shcherbatov Message-ID: <0bfa59b1-1d13-9237-9db8-79dca958d4f4@tarantool.org> Date: Tue, 2 Apr 2019 18:49:24 +0300 MIME-Version: 1.0 In-Reply-To: <20190328102121.3arq2o4iflksxsio@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: Hi! Thank you for such a detailed answer! > The helper returns true iff multikey_len equals path hence the bool > return value is useless. Return multikey offset instead. And rename > the function appropriately. Refactored as json_path_multikey_offset helper. /** * Scan the JSON path string and return the offset of the first * character [*] (the array index placeholder). * - if [*] is not found, -1 is returned. * - specified JSON path must be valid * (may be tested with json_path_validate). */ int json_path_multikey_offset(const char *path, int path_len, int index_base); > This flag is ugly, both semantically and esthetically. As a matter fact, > I don't see why you would need such a function at all. You can check if > paths to the first multikey part is the same right in key_def_new. Now there is no such helper. > > BTW I don't like that this check lives in index_def_is_valid, because it > means that there may be a key_def with invalid configuration. Besides, > this makes checks scattered all over the code, making it difficult to > follow. Ok, I've moved it to key_def_new, in key_def_set_part. But this led to the fact that key_def_set_part should now return the error that made the code more complicated. > There's a lot of code duplication between vy_stmt_new_surrogate_delete and > tuple_create_field_map - you should try to eliminate it before extending > field map. I've introduced a new tuple_parse_iterator class that simplify this code. > tuple_compare and tuple_compare_with_key don't make any sense for > multikey indexes. You should add stubs for them. I guess the stubs > should simply assert(0). The same's fair for extract_key and hints. >> @@ -621,7 +695,8 @@ memtx_tree_index_create_iterator(struct index *base, enum iterator_type type, >> it->type = type; >> it->key_data.key = key; >> it->key_data.part_count = part_count; >> - it->key_data.hint = key_hint(key, part_count, cmp_def); >> + if (!key_def_is_multikey(cmp_def)) >> + it->key_data.hint = key_hint(key, part_count, cmp_def); > > Please get rid of this 'if'. Overload key_hint instead. In a patchset I just sent you, pay attention to key_hint's stub. It does not contain unreachable() checks, because these functions are invoked (in contrast to other stubs); It allows not to rewrite memtx_tree_index_get and memtx_tree_index_create_iterator.