[tarantool-patches] Re: [PATCH v3 3/4] box: introduce JSON indexes
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Sep 3 13:32:46 MSK 2018
Thanks for the fixes! See 28 comments below and a commit
on the branch.
1. On your branch vinyl/errinj and vinyl/info are failing.
I do not know is it because you did not cherry-pick
Vladimir's patch or because of a bug in your patch, but it
should not fail.
This is what I get sometimes on engine/null test:
[008] Test failed! Result content mismatch:
[008] --- engine/null.result Thu Aug 30 15:20:24 2018
[008] +++ engine/null.reject Sat Sep 1 12:51:29 2018
[008] @@ -630,7 +630,7 @@
[008] ...
[008] box.snapshot()
[008] ---
[008] -- ok
[008] +- error: 'Invalid VYLOG file: Bad record: failed to decode index key definition'
> diff --git a/src/box/key_def.c b/src/box/key_def.c
> index 440d41e..cf1169e 100644
> --- a/src/box/key_def.c
> +++ b/src/box/key_def.c> @@ -241,6 +291,13 @@ key_part_cmp(const struct key_part *parts1, uint32_t part_count1,
> if (part1->is_nullable != part2->is_nullable)
> return part1->is_nullable <
> part2->is_nullable ? -1 : 1;
> + /* Lexicographic strings order. */
> + if (part1->path_len != part2->path_len)
> + return part1->path_len - part2->path_len;
> + int rc = 0;
> + if ((rc = memcmp(part1->path, part2->path,
> + part1->path_len)) != 0)
> + return rc;
2. Your question:
> I've checked this code reachable with assert. But I don't now how handle it manually..
> Please, help.
Answer: you should write a test, that extends a path in such
a way that newly indexed field values change order of index
sorting. Select from the index should return the new order.
Before you fixed this thing it returned an old order. Test it,
please.
> }
> return part_count1 < part_count2 ? -1 : part_count1 > part_count2;
> }
> @@ -284,7 +353,9 @@ key_def_update_optionality(struct key_def *def, uint32_t min_field_count)
> for (uint32_t i = 0; i < def->part_count; ++i) {
> struct key_part *part = &def->parts[i];
> def->has_optional_parts |= part->is_nullable &&
> - min_field_count < part->fieldno + 1;
> + part->path == NULL &&
> + (min_field_count <
> + part->fieldno + 1);
3. Why does path != NULL guarantee that a def has no optional parts?
On the contrary, it means that a part is optional if it is not flat
and is nullable.
What is funny, even now in all has_optional_parts usage places you
check both for is_flat and has_optional_parts.
> /*
> * One optional part is enough to switch to new
> * comparators.
> @@ -432,10 +524,113 @@ key_def_decode_parts_166(struct key_part_def *parts, uint32_t part_count,
> fields[part->fieldno].is_nullable :
> key_part_def_default.is_nullable);
> part->coll_id = COLL_NONE;
> + part->path = NULL;
> }
> return 0;
> }
>
> +/**
> + * Verify key_part JSON path and convert to canonical form.
> + *
> + * @param region Region to make allocations.
> + * @param part Part with path to update.
> + * @param path_extra Extra allocated space to reuse if possible.
> + * @param path_extra_size The @path_extra size.
> + *
> + * @retval -1 on error.
> + * @retval 0 on success.
> + */
> +static int
> +key_def_normalize_json_path(struct region *region, struct key_part_def *part,
> + char **path_extra, uint32_t *path_extra_size)
> +{
> + const char *err_msg = NULL;
4. You do not need announcement of this variable.
(fixed by me)
> +
> + uint32_t allocated_size = *path_extra_size;
> + char *path = *path_extra;
> +
> + uint32_t path_len = strlen(part->path);
> + struct json_path_parser parser;
> + struct json_path_node node;
> + json_path_parser_create(&parser, part->path, path_len);
> + /*
> + * A worst-case scenario is .a -> ["a"]
> + * i.e. 2.5 * path_len + 1 is enough.
> + */
> + uint32_t new_path_size = 2.5 * path_len + 1;
> + if (new_path_size >= allocated_size) {
> + path = region_alloc(region, new_path_size);
> + if (path == NULL) {
> + diag_set(OutOfMemory, new_path_size,
> + "region_alloc", "path");
> + return -1;
> + }
> + allocated_size = new_path_size;
> + }
> + assert(path != NULL);
> + part->path = path;
> + int rc = json_path_next(&parser, &node);
> + if (rc != 0)
> + goto error_invalid_json;
> + if (node.type != JSON_PATH_NUM) {
> + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
> + part->fieldno,
> + "invalid JSON path: first part should "
> + "be defined as array index");
> + return -1;
> + }
> + if (node.num - TUPLE_INDEX_BASE != part->fieldno) {
> + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
> + part->fieldno,
> + "invalid JSON path: first part refers "
> + "to invalid field");
> + return -1;
> + }
> + uint32_t lexemes = 0;
> + do {
> + if (node.type == JSON_PATH_NUM) {
> + path += sprintf(path, "[%u]", (uint32_t) node.num);
> + } else if (node.type == JSON_PATH_STR) {
> + path += sprintf(path, "[\"%.*s\"]", node.len, node.str);
> + } else {
> + unreachable();
> + }
> + lexemes++;
> + } while ((rc = json_path_next(&parser, &node)) == 0 &&
> + node.type != JSON_PATH_END);
> + if (rc != 0 || node.type != JSON_PATH_END)
> + goto error_invalid_json;
> + if (lexemes == 1) {
> + /* JSON index is useless. */
> + path = part->path;
> + part->path = NULL;
5. As I remember, I asked you to use gcov to see
which code is untested and test it. But looks like
you did not, but I did and here a result is:
638: 602: if (lexemes == 1) {
-: 603: /* JSON index is useless. */
#####: 604: path = part->path;
#####: 605: part->path = NULL;
#####: 606: } else {
Either JSON path is never useless in the tests or
there is a bug. Please, fix.
> + } else {
> + /* Skip terminating zero. */
> + path++;
> + /* Account constructed string size. */
> + allocated_size -= path - part->path;
> + }
> @@ -552,24 +767,37 @@ key_def_merge(const struct key_def *first, const struct key_def *second)
> new_def->is_nullable = first->is_nullable || second->is_nullable;
> new_def->has_optional_parts = first->has_optional_parts ||
> second->has_optional_parts;
> + /* Path data write position in the new key_def. */
> + char *data = (char *)new_def + key_def_sizeof(new_part_count, 0);
> /* Write position in the new key def. */
> uint32_t pos = 0;
> /* Append first key def's parts to the new index_def. */
> part = first->parts;
> end = part + first->part_count;
> for (; part != end; part++) {
> + if (part->path != NULL) {
> + new_def->parts[pos].path = data;
> + data += part->path_len + 1;
> + }
> key_def_set_part(new_def, pos++, part->fieldno, part->type,
> - part->is_nullable, part->coll, part->coll_id);
> + part->is_nullable, part->coll, part->coll_id,
> + part->path, part->path_len);
> }
>
> /* Set-append second key def's part to the new key def. */
> part = second->parts;
> end = part + second->part_count;
> for (; part != end; part++) {
> - if (key_def_find(first, part->fieldno))
> + if (key_def_find(first, part->fieldno, part->path,
> + part->path_len) != NULL)
> continue;
> + if (part->path != NULL) {
> + new_def->parts[pos].path = data;
> + data += part->path_len + 1;
> + }
> key_def_set_part(new_def, pos++, part->fieldno, part->type,
> - part->is_nullable, part->coll, part->coll_id);
> + part->is_nullable, part->coll, part->coll_id,
> + part->path, part->path_len);
6. Here and above you should set epoch and slot offset.
> }
> return new_def;
> }
> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index 5d7df4d..9dded75 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -1063,14 +1077,14 @@ tuple_compare_create(const struct key_def *def)
> return tuple_compare_sequential<true, true>;
> else
> return tuple_compare_sequential<true, false>;
> - } else if (def->has_optional_parts) {
> - return tuple_compare_slowpath<true, true, false>;
> } else {
> - return tuple_compare_slowpath<true, false, false>;
> + int func_idx = 1 + 2 * def->has_optional_parts +
> + 4 * def->has_json_paths;
> + return compare_slowpath_funcs[func_idx];
> }
> }
> assert(! def->has_optional_parts);
> - if (!key_def_has_collation(def)) {
> + if (!key_def_has_collation(def) && !def->has_json_paths) {
> /* Precalculated comparators don't use collation */
> for (uint32_t k = 0;
> k < sizeof(cmp_arr) / sizeof(cmp_arr[0]); k++) {
> @@ -1088,6 +1102,8 @@ tuple_compare_create(const struct key_def *def)
> }
> if (key_def_is_sequential(def))
> return tuple_compare_sequential<false, false>;
> + else if (def->has_json_paths)
> + return tuple_compare_slowpath<false, false, true>;
> else
> return tuple_compare_slowpath<false, false, false>;
7. Comparators matrix is useless unless you use it for all
tuple_compare_slowpath usage places.
The same for tuple_compare_with_key.
> }
> @@ -1283,14 +1310,15 @@ tuple_compare_with_key_create(const struct key_def *def)
> return tuple_compare_with_key_sequential<true,
> false>;
> }
> - } else if (def->has_optional_parts) {
> - return tuple_compare_with_key_slowpath<true, true, false>;
> } else {
> - return tuple_compare_with_key_slowpath<true, false, false>;
> + int func_idx = (def->is_nullable ? 1 : 0) +
> + 2 * (def->has_optional_parts ? 1 : 0) +
> + 4 * (def->has_json_paths ? 1 : 0);
> + return compare_with_key_slowpath_funcs[func_idx];
> }
> }
> assert(! def->has_optional_parts);
> - if (!key_def_has_collation(def)) {
> + if (!key_def_has_collation(def) && !def->has_json_paths) {
> /* Precalculated comparators don't use collation */
> for (uint32_t k = 0;
> k < sizeof(cmp_wk_arr) / sizeof(cmp_wk_arr[0]);
> @@ -1311,6 +1339,8 @@ tuple_compare_with_key_create(const struct key_def *def)
> }
> if (key_def_is_sequential(def))
> return tuple_compare_with_key_sequential<false, false>;
> + else if (def->has_json_paths)
> + return tuple_compare_with_key_slowpath<false, false, true>;
> else
> return tuple_compare_with_key_slowpath<false, false, false>;
> }
> diff --git a/src/box/tuple_extract_key.cc b/src/box/tuple_extract_key.cc
> index 2ea405a..2ee399f 100644
> --- a/src/box/tuple_extract_key.cc
> +++ b/src/box/tuple_extract_key.cc
> @@ -1,15 +1,31 @@
> #include "tuple_extract_key.h"
> #include "tuple.h"
> #include "fiber.h"
> +#include "json/path.h"
>
> enum { MSGPACK_NULL = 0xc0 };
>
> +/** True if key part i and i+1 are suquential. */
8. Typo.
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 2d4a85f..363fdff 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -30,6 +30,7 @@
> */
> #include "json/path.h"
> #include "tuple_format.h"
> +#include "assoc.h"
>
> /** Global table of tuple formats */
> struct tuple_format **tuple_formats;
> @@ -38,10 +39,478 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL;
> static uint32_t formats_size = 0, formats_capacity = 0;
>
> static const struct tuple_field tuple_field_default = {
> - FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, false,
> + FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, false, {{NULL, 0}}
> };
>
> /**
> + * Propagate @a field to MessagePack(field)[key].
> + * @param[in][out] field Field to propagate.
> + * @param key Key to propagate to.
> + * @param len Length of @a key.
> + * @param field_idx Field index in map.
> + *
> + * @retval 0 Success, the index was found.
> + * @retval -1 Not found.
> + */
> +static inline int
> +tuple_field_go_to_key(const char **field, const char *key, int len,
> + uint32_t *field_idx)
> +{
> + enum mp_type type = mp_typeof(**field);
> + if (type != MP_MAP)
> + return -1;
> + uint32_t count = mp_decode_map(field);
> + for (uint32_t idx = 0; idx < count; idx++) {
> + type = mp_typeof(**field);
> + if (type == MP_STR) {
> + uint32_t value_len;
> + const char *value = mp_decode_str(field, &value_len);
> + if (value_len == (uint)len &&
> + memcmp(value, key, len) == 0) {
> + *field_idx = idx;
> + return 0;
> + }
> + } else {
> + /* Skip key. */
> + mp_next(field);
9. Never tested:
167: 73: } else {
-: 74: /* Skip key. */
#####: 75: mp_next(field);
-: 76: }
> + }
> + /* Skip value. */
> + mp_next(field);
> + }
> + return -1;
> +}
> +/**
> + * Construct field tree level for JSON path part.
> + *
> + * @param[in, out] tuple_field Pointer to record to start with
> + * would be changed to record that math
> + * @part lexeme.
> + * @param fieldno Number of root space field.
> + * @param part JSON path lexeme to represent in field tree.
> + * @retval -1 On error.
> + * @retval 0 On success.
> + */
> +static int
> +json_field_tree_append(struct tuple_field **field_subtree, uint32_t fieldno,
> + struct json_path_node *part)
> +{
> + enum field_type type;
> + struct tuple_field *field = *field_subtree;
> + switch (part->type) {
> + case JSON_PATH_NUM: {
> + type = FIELD_TYPE_ARRAY;
> + if (field->type != FIELD_TYPE_ANY && field->type != type)
> + goto error_type_mistmatch;
10. The same.
668: 180: type = FIELD_TYPE_ARRAY;
1020: 181: if (field->type != FIELD_TYPE_ANY && field->type != type)
#####: 182: goto error_type_mistmatch;
-: 183: /* Create or resize field array if required. */
> +int
> +tuple_field_bypass_and_init(const struct tuple_field *field, uint32_t idx,
> + const char *tuple, const char **offset,
> + uint32_t *field_map)
> +{
> + assert(offset != NULL);
> + int rc = 0;
> + const char *mp_data = *offset;
> + const char *valid_type_str = NULL;
> + const char *err = NULL;
> + enum mp_type type = mp_typeof(**offset);
> + if (field->type == FIELD_TYPE_MAP) {
> + if (type != MP_MAP) {
> + valid_type_str = mp_type_strs[MP_MAP];
> + goto error_type_mistmatch;
> + }
> + const char *max_offset = *offset;
> + uint32_t max_idx = 0;
> + uint32_t count = mp_decode_map(&max_offset);
> + mh_int_t i;
> + mh_foreach(field->map, i) {
> + struct mh_strnptr_node_t *ht_record =
> + mh_strnptr_node(field->map, i);
> + struct tuple_field *leaf = ht_record->val;
> + assert(leaf != NULL);
> +
> + const char *raw = *offset;
> + uint32_t map_idx = 0;
> + rc = tuple_field_go_to_key(&raw, ht_record->str,
> + (int)ht_record->len,
> + &map_idx);
> + if (rc != 0 && !leaf->is_nullable) {
> + err = tt_sprintf("map doesn't contain key "
> + "'%.*s' defined in index",
> + ht_record->len,ht_record->str);
> + goto error_invalid_document;
> + }
> + if (rc != 0) {
> + if (field_map != NULL &&
> + leaf->offset_slot != TUPLE_OFFSET_SLOT_NIL)
> + field_map[leaf->offset_slot] = 0;
> + continue;
> + }
> + if (tuple_field_bypass_and_init(leaf, idx, tuple, &raw,
> + field_map) != 0)
> + return -1;
> + max_idx = MAX(max_idx, map_idx + 1);
> + max_offset = MAX(max_offset, raw);
> + }
> + *offset = max_offset;
> + while (count-- > max_idx) {
> + mp_next(offset);
> + mp_next(offset);
> + }
> + return 0;
> + } else if (field->type == FIELD_TYPE_ARRAY) {
> + if (type != MP_ARRAY) {
> + valid_type_str = mp_type_strs[MP_ARRAY];
> + goto error_type_mistmatch;
> + }
11.
132: 346: } else if (field->type == FIELD_TYPE_ARRAY) {
458: 347: if (type != MP_ARRAY) {
193: 348: valid_type_str = mp_type_strs[MP_ARRAY];
#####: 349: goto error_type_mistmatch;
#####: 350: }
-: 351: uint32_t count = mp_decode_array(offset);
> + uint32_t count = mp_decode_array(offset);
> + for (uint32_t i = count; i < field->array_size; i++) {
> + /*
> + * Index fields out of document array
> + * must be nullable.
> + */
> + struct tuple_field *leaf = field->array[i];
> + if (leaf == NULL)
> + continue;
> + if (leaf->is_nullable) {
> + if (field_map != NULL &&
> + leaf->offset_slot != TUPLE_OFFSET_SLOT_NIL)
> + field_map[leaf->offset_slot] = 0;
> + continue;
> + }
> + err = tt_sprintf("array size %d is less than item %d "
> + "defined in index", i,
> + field->array_size);
> + goto error_invalid_document;
12.
34: 365: }
-: 366: err = tt_sprintf("array size %d is less than item %d "
#####: 367: "defined in index", i,
#####: 368: field->array_size);
#####: 369: goto error_invalid_document;
#####: 370: }
> @@ -63,12 +532,18 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
> format->fields[i].type = fields[i].type;
> format->fields[i].offset_slot = TUPLE_OFFSET_SLOT_NIL;
> format->fields[i].is_nullable = fields[i].is_nullable;
> + /* Don't need to init format->fields[i].map. */
> + format->fields[i].childs = NULL;
> + format->fields[i].array_size = 0;
13. Auxiliary 'childs' makes no sense if you still need to
init array_size. Please, come up with a better solution.
Inlined memset from offsetof is not good as well. Or it
could be wrapped into functions which test for childs,
nullify them.
> }
> /* Initialize remaining fields */
> for (uint32_t i = field_count; i < format->field_count; i++)
> format->fields[i] = tuple_field_default;
>
> int current_slot = 0;
> + /* Memory allocated for JSON paths if any. */
> + char *data = (char *)format + sizeof(struct tuple_format) +
> + format->field_count * sizeof(struct tuple_field);
>
> /* extract field type info */
> for (uint16_t key_no = 0; key_no < key_count; ++key_no) {
> @@ -91,6 +566,24 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
> field->is_nullable = false;
> }
>
> + if (part->path != NULL) {
> + field->is_key_part = true;
> + assert(is_sequential == false);
> + struct tuple_field *leaf = NULL;
> + if (tuple_format_add_json_path(format,
> + part->path,
> + part->path_len,
> + part->path_hash,
> + part->type,
> + part->is_nullable,
14. Out of 80.
> + &data,
> + &leaf) != 0)
> + return -1;
> + assert(leaf != NULL);
> + if (leaf->offset_slot == TUPLE_OFFSET_SLOT_NIL)
> + leaf->offset_slot = --current_slot;
> + continue;
15. Why do you check for path !=/== NULL in the code below if
you already know it is == NULL? The only point of moving non-flat
part processing there was to remove those checks.
(fixed by me)
16. I see that you init field->is_nullable before checking
for path != NULL, but it is wrong. If a field is not flat,
then its first level nullability could be != a leaf
nullability. Now it is impossible due to lack of non-flat
format, but it will be. Besides, I think you should
move all this cycle body into a function like
tuple_format_add_key_part or something.
17. For flat fields different nullability is allowed
and the strictest is chosen. Why do you forbid it for
non-flat fields (in tuple_format_add_json_path)?
The same for compatible types. Below they are allowed,
but for non-flat fields are not.
> + }
> /*
> * Check that there are no conflicts
> * between index part types and space
> @@ -100,10 +593,12 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
> * used in tuple_format.
> */
> if (field_type1_contains_type2(field->type,
> - part->type)) {
> + part->type) &&
> + part->path == NULL) {
> field->type = part->type;
> } else if (! field_type1_contains_type2(part->type,
> - field->type)) {
> + field->type) &&
> + part->path == NULL) {
> const char *name;
> int fieldno = part->fieldno + TUPLE_INDEX_BASE;
> if (part->fieldno >= field_count) {
> @@ -201,19 +696,47 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count,
> uint32_t space_field_count, struct tuple_dictionary *dict)
> {
> uint32_t index_field_count = 0;
> + /* JSON path hashtable. */
> + struct mh_strnptr_t *path_hash = json_path_hash_create(0);
> + if (path_hash == NULL)
> + return NULL;
> /* find max max field no */
> for (uint16_t key_no = 0; key_no < key_count; ++key_no) {
> const struct key_def *key_def = keys[key_no];
> const struct key_part *part = key_def->parts;
> const struct key_part *pend = part + key_def->part_count;
> for (; part < pend; part++) {
> + if (part->path != NULL &&
> + json_path_hash_insert(path_hash, part->path,
> + part->path_len, NULL) != 0) {
> + json_path_hash_delete(path_hash);
> + return NULL;
> + }
> index_field_count = MAX(index_field_count,
> part->fieldno + 1);
> }
> }
> + size_t extra_size = 0;
> + if (mh_size(path_hash) == 0) {
> + /* Hashtable is useless. */
> + json_path_hash_delete(path_hash);
> + path_hash = NULL;
> + } else {
> + /*
> + * Calculate unique JSON paths count.
> + * Path data would be copied later on
> + * tuple_format_create routine.
> + */
> + mh_int_t i;
> + mh_foreach(path_hash, i) {
> + struct mh_strnptr_node_t *node =
> + mh_strnptr_node(path_hash, i);
> + extra_size += node->len + 1;
> + }
> + }
> uint32_t field_count = MAX(space_field_count, index_field_count);
> uint32_t total = sizeof(struct tuple_format) +
> - field_count * sizeof(struct tuple_field);
> + field_count * sizeof(struct tuple_field) + extra_size;
>
> struct tuple_format *format = (struct tuple_format *) malloc(total);
> if (format == NULL) {
18. Two leaks of path_hash below where 'return NULL' is.
(fixed by me)
> @@ -377,18 +945,14 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map,
> return -1;
> }
>
> - /* first field is simply accessible, so we do not store offset to it */
> - enum mp_type mp_type = mp_typeof(*pos);
> + /*
> + * First field is simply accessible, store offset to it
> + * only for JSON path.
> + */
19. The comment now is not linked with the code below.
(fixed by me)
> + uint32_t i = 0;
> + enum mp_type mp_type;
> const struct tuple_field *field = &format->fields[0];
> - if (key_mp_type_validate(field->type, mp_type, ER_FIELD_TYPE,
> - TUPLE_INDEX_BASE, field->is_nullable))
> - return -1;
> - mp_next(&pos);
> - /* other fields...*/
> - ++field;
> - uint32_t i = 1;
> - uint32_t defined_field_count = MIN(field_count, format->field_count);
> - if (field_count < format->index_field_count) {
> + if (field_count < format->index_field_count || field->childs != NULL) {
> /*
> * Nullify field map to be able to detect by 0,
> * which key fields are absent in tuple_field().
> @@ -512,55 +1090,106 @@ tuple_field_go_to_index(const char **field, uint64_t index)
> return -1;
> }
>
> -/**
> - * Propagate @a field to MessagePack(field)[key].
> - * @param[in][out] field Field to propagate.
> - * @param key Key to propagate to.
> - * @param len Length of @a key.
> - *
> - * @retval 0 Success, the index was found.
> - * @retval -1 Not found.
> - */
> -static inline int
> -tuple_field_go_to_key(const char **field, const char *key, int len)
> +const char *
> +tuple_field_by_part_raw(const struct tuple_format *format, const char *data,
> + const uint32_t *field_map, struct key_part *part)
> {
> - enum mp_type type = mp_typeof(**field);
> - if (type != MP_MAP)
> - return -1;
> - uint64_t count = mp_decode_map(field);
> - for (; count > 0; --count) {
> - type = mp_typeof(**field);
> - if (type == MP_STR) {
> - uint32_t value_len;
> - const char *value = mp_decode_str(field, &value_len);
> - if (value_len == (uint)len &&
> - memcmp(value, key, len) == 0)
> - return 0;
> - } else {
> - /* Skip key. */
> - mp_next(field);
> - }
> - /* Skip value. */
> - mp_next(field);
> + if (likely(part->path == NULL))
> + return tuple_field_raw(format, data, field_map, part->fieldno);
> +
> + struct mh_strnptr_node_t *ht_record = NULL;
> + int32_t offset_slot;
> + if (likely(part->offset_slot_epoch == format->epoch)) {
> + offset_slot = part->offset_slot;
20. This code is different on the branch and in the email. Here it is:
> if (likely(part->offset_slot_epoch == format->epoch &&
> format->epoch != 0)) {
> offset_slot = part->offset_slot;
And please explain how format epoch can be 0 ever? A format epoch is
initialized by 0 but then it is always either reset to an old epoch or
old epoch + 1. When a space has no indexes, it could be 0, but each
new index alters the space and updates its epoch. So when a format
epoch is 0, it has no tuples.
> + } else if (format->path_hash != NULL &&
> + (ht_record = json_path_hash_get(format->path_hash, part->path,
> + part->path_len,
> + part->path_hash)) != NULL) {
> + struct tuple_field *field = ht_record->val;
> + assert(field != NULL);
> + offset_slot = field->offset_slot;
> + } else {
> + /*
> + * Legacy tuple having no field map for
> + * JSON index.
> + */
> + uint32_t path_hash =
> + field_name_hash(part->path, part->path_len);
> + const char *raw = NULL;
> + if (tuple_field_raw_by_path(format, data, field_map,
> + part->path, part->path_len,
> + path_hash, &raw) != 0)
> + raw = NULL;
> + return raw;
> }
> - return -1;
> + assert(offset_slot < 0);
> + assert(-offset_slot * sizeof(uint32_t) <= format->field_map_size);
> + if (unlikely(field_map[offset_slot] == 0))
> + return NULL;
21. Why do not you update epoch and slot in such a case?
> + /* Cache offset_slot if required. */
> + if (part->offset_slot_epoch < format->epoch) {
22. This check is not needed for the best case when epochs are
equal in format and key part. Please, move it to the only place
where it is needed above.
23. By the way, why do you store offset_slot_epoch in key_part
instead of in a parent key_def? As I understand, it is the same
for all parts of a key_def.
> + part->offset_slot = offset_slot;
> + part->offset_slot_epoch = format->epoch;
> + }
> + return data + field_map[offset_slot];
> }
>
> -const char *
> -tuple_field_by_part_raw(const struct tuple_format *format, const char *data,
> - const uint32_t *field_map, struct key_part *part)
> +int
> +tuple_field_dig_with_parser(struct json_path_parser *parser, const char **field)
> {
> - return tuple_field_raw(format, data, field_map, part->fieldno);
> + int rc;
> + struct json_path_node node;
> + while ((rc = json_path_next(parser, &node)) == 0) {
> + uint32_t dummy;
> + switch(node.type) {
> + case JSON_PATH_NUM:
> + rc = tuple_field_go_to_index(field, node.num);
> + break;
> + case JSON_PATH_STR:
> + rc = tuple_field_go_to_key(field, node.str,
> + node.len, &dummy);
> + break;
> + default:
> + assert(node.type == JSON_PATH_END);
> + return 0;
> + }
> + if (rc != 0) {
> + *field = NULL;
> + return 0;
> + }
> + }
> + return rc;
> }
>
> int
> -tuple_field_raw_by_path(struct tuple_format *format, const char *tuple,
> +tuple_field_raw_by_path(const struct tuple_format *format, const char *tuple,
> const uint32_t *field_map, const char *path,
> uint32_t path_len, uint32_t path_hash,
> const char **field)
> {
> assert(path_len > 0);
> uint32_t fieldno;
> + if (format->path_hash != NULL) {
> + /*
> + * The path hash for format->path_hash hashtable
> + * may may be different from path_hash specified
> + * as function argument.
> + */
> + struct mh_strnptr_node_t *ht_record =
> + json_path_hash_get(format->path_hash, path, path_len,
> + mh_strn_hash(path, path_len));
> + if (ht_record != NULL) {
> + struct tuple_field *leaf = ht_record->val;
> + assert(leaf != NULL);
> + int32_t offset_slot = leaf->offset_slot;
> + assert(offset_slot != TUPLE_OFFSET_SLOT_NIL);
> + if (field_map[offset_slot] != 0)
> + *field = tuple + field_map[offset_slot];
> + else
> + *field = NULL;
> + return 0;
> + }
24. Untested.
186: 1171: struct mh_strnptr_node_t *ht_record =
372: 1172: json_path_hash_get(format->path_hash, path, path_len,
186: 1173: mh_strn_hash(path, path_len));
186: 1174: if (ht_record != NULL) {
#####: 1175: struct tuple_field *leaf = ht_record->val;
#####: 1176: assert(leaf != NULL);
#####: 1177: int32_t offset_slot = leaf->offset_slot;
#####: 1178: assert(offset_slot != TUPLE_OFFSET_SLOT_NIL);
#####: 1179: if (field_map[offset_slot] != 0)
#####: 1180: *field = tuple + field_map[offset_slot];
-: 1181: else
#####: 1182: *field = NULL;
#####: 1183: return 0;
-: 1184: }
186: 1185: }
> + }
> /*
> * It is possible, that a field has a name as
> * well-formatted JSON. For example 'a.b.c.d' or '[1]' can
> diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
> index ecbf64c..af81a58 100644
> --- a/src/box/tuple_format.h
> +++ b/src/box/tuple_format.h
> @@ -419,11 +439,49 @@ tuple_field_raw_by_name(struct tuple_format *format, const char *tuple,
> * @retval -1 Error in JSON path.
> */
> int
> -tuple_field_raw_by_path(struct tuple_format *format, const char *tuple,
> +tuple_field_raw_by_path(const struct tuple_format *format, const char *tuple,
> const uint32_t *field_map, const char *path,
> uint32_t path_len, uint32_t path_hash,
> const char **field);
>
> +/**
> + * Retrieve document data @field with initialized @parser.
> + * @param parser JSON parser.
> + * @param[in, out] field Tuple field to lookup.
> + * @retval 0 On success.
> + * @retval > 0 On error in path been used to initialize @parser.
> + */
> +int
> +tuple_field_dig_with_parser(struct json_path_parser *parser,
> + const char **field);
> +
> +/**
> + * Get @hashtable record by key @path, @path_len.
> + * @param hashtable Storage to lookup.
> + * @param path Path string.
> + * @param path_len Length of @path.
25. path_hash parameter missed.
> + * @retval NULL On nothing found.
> + * @retval not NULL Leaf field pointer for registered path.
> + */
> +struct mh_strnptr_node_t *
> +json_path_hash_get(struct mh_strnptr_t *hashtable, const char *path,
> + uint32_t path_len, uint32_t path_hash);
> +
> +/**
> + * Observe JSON path tree in @field comparing with @tuple
> + * structure. Initialize field map if specified.
> + * @param field Field to use on initialization.
> + * @param idx Root field index to emmit correct error.
> + * @param tuple Source raw data.
26. offset parameter missed.
> + * @param field_map Field map to initialize (optional).
> + * @retval 0 On success.
> + * @retval -1 On error.
> + */
> +int
> +tuple_field_bypass_and_init(const struct tuple_field *field, uint32_t idx,
> + const char *tuple, const char **offset,
> + uint32_t *field_map);
> +
> #if defined(__cplusplus)
> } /* extern "C" */
> diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
> index 37da282..6b452cd 100644
> --- a/src/box/vy_stmt.c
> +++ b/src/box/vy_stmt.c
> @@ -330,6 +331,67 @@ vy_stmt_replace_from_upsert(const struct tuple *upsert)
> return replace;
> }
>
> +static void
> +vy_stmt_msgpack_build(struct tuple_field *field, char *tuple,
> + uint32_t *field_map, char **offset, bool write_data)
> +{
> + if (field->type == FIELD_TYPE_ARRAY) {
> + if (write_data)
> + *offset = mp_encode_array(*offset, field->array_size);
> + else
> + *offset += mp_sizeof_array(field->array_size);
> + for (uint32_t i = 0; i < field->array_size; i++) {
> + if (field->array[i] == NULL) {
> + if (write_data)
> + *offset = mp_encode_nil(*offset);
> + else
> + *offset += mp_sizeof_nil();
> + continue;
> + }
> + vy_stmt_msgpack_build(field->array[i], tuple, field_map,
> + offset, write_data);
> + }
> + return;
> + } else if (field->type == FIELD_TYPE_MAP) {
> + if (write_data)
> + *offset = mp_encode_map(*offset, mh_size(field->map));
> + else
> + *offset += mp_sizeof_map(mh_size(field->map));
> + mh_int_t i;
> + mh_foreach(field->map, i) {
> + struct mh_strnptr_node_t *node =
> + mh_strnptr_node(field->map, i);
> + assert(node);
> + if (write_data) {
> + *offset = mp_encode_str(*offset, node->str,
> + node->len);
> + } else {
> + *offset += mp_sizeof_str(node->len);
> + }
> + vy_stmt_msgpack_build(node->val, tuple, field_map,
> + offset, write_data);
> + }
> + return;
> + }
> +
> + struct iovec *iov = field->arg;
> + if (iov == NULL) {
> + if (write_data)
> + *offset = mp_encode_nil(*offset);
> + else
> + *offset += mp_sizeof_nil();
> + } else {
> + if (write_data) {
> + uint32_t data_offset = *offset - tuple;
> + memcpy(*offset, iov->iov_base, iov->iov_len);
> + field->arg = NULL;
> + if (field->offset_slot != TUPLE_OFFSET_SLOT_NIL)
> + field_map[field->offset_slot] = data_offset;
> + }
> + *offset += iov->iov_len;
> + }
> +}
27. I still think that your walker should be generic and it is not
ok that you reimplement it when a task is slightly extended, but it
is up to Vova.
28. Please, remove tuple_field->arg field. It is extra ugly. Find a
better solution. Why not just make it the builder's function argument?
> +
> static struct tuple *
> vy_stmt_new_surrogate_from_key(const char *key, enum iproto_type type,
> const struct key_def *cmp_def,
More information about the Tarantool-patches
mailing list