[tarantool-patches] Re: [PATCH v2 0/5] box: introduce multikey indexes in memtx

[Kirill Shcherbatov]

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.