From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v3 3/4] box: introduce JSON indexes Date: Mon, 3 Sep 2018 13:35:52 +0300 [thread overview] Message-ID: <20da5c2d-1eec-f951-807f-00f2e6873bcd@tarantool.org> (raw) In-Reply-To: <cc078d7c-4ce3-ec9f-b26f-990edd6db844@tarantool.org> Forgot to send diff: commit deedcb31040cfbe5b93393e54ec55f3cde5631b8 Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Date: Sat Sep 1 13:18:52 2018 -0300 Review fixes diff --git a/src/box/key_def.c b/src/box/key_def.c index cf1169ea0..ea7380b39 100644 --- a/src/box/key_def.c +++ b/src/box/key_def.c @@ -544,8 +544,6 @@ 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; - uint32_t allocated_size = *path_extra_size; char *path = *path_extra; @@ -589,7 +587,8 @@ key_def_normalize_json_path(struct region *region, struct key_part_def *part, uint32_t lexemes = 0; do { if (node.type == JSON_PATH_NUM) { - path += sprintf(path, "[%u]", (uint32_t) node.num); + path += sprintf(path, "[%llu]", + (unsigned long long) node.num); } else if (node.type == JSON_PATH_STR) { path += sprintf(path, "[\"%.*s\"]", node.len, node.str); } else { @@ -622,10 +621,11 @@ key_def_normalize_json_path(struct region *region, struct key_part_def *part, } return 0; -error_invalid_json: - err_msg = tt_sprintf("invalid JSON path '%.*s': path has invalid " - "structure (error at position %d)", parser.src_len, - parser.src, rc); +error_invalid_json: ; + const char *err_msg = + tt_sprintf("invalid JSON path '%.*s': path has invalid "\ + "structure (error at position %d)", parser.src_len, + parser.src, rc); diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, part->fieldno + TUPLE_INDEX_BASE, err_msg); return -1; @@ -704,19 +704,20 @@ key_def_decode_parts_160(struct key_part_def *parts, uint32_t part_count, return 0; } -const struct key_part * -key_def_find(const struct key_def *key_def, uint32_t fieldno, const char *path, - uint32_t path_len) +bool +key_def_contains_part(const struct key_def *key_def, + const struct key_part *to_find) { const struct key_part *part = key_def->parts; const struct key_part *end = part + key_def->part_count; for (; part != end; part++) { - if (part->fieldno == fieldno && part->path_len == path_len && - (part->path == NULL || - memcmp(part->path, path, path_len) == 0)) - return part; + if (part->fieldno == to_find->fieldno && + part->path_len == to_find->path_len && + (part->path == NULL || memcmp(part->path, to_find->path, + to_find->path_len) == 0)) + return true; } - return NULL; + return false; } bool @@ -725,8 +726,7 @@ key_def_contains(const struct key_def *first, const struct key_def *second) const struct key_part *part = second->parts; const struct key_part *end = part + second->part_count; for (; part != end; part++) { - if (key_def_find(first, part->fieldno, part->path, - part->path_len) == NULL) + if (! key_def_contains_part(first, part)) return false; } return true; @@ -750,8 +750,7 @@ key_def_merge(const struct key_def *first, const struct key_def *second) part = second->parts; end = part + second->part_count; for (; part != end; part++) { - if (key_def_find(first, part->fieldno, part->path, - part->path_len) != NULL) + if (key_def_contains_part(first, part)) --new_part_count; else if (part->path != NULL) sz += part->path_len + 1; @@ -788,8 +787,7 @@ key_def_merge(const struct key_def *first, const struct key_def *second) part = second->parts; end = part + second->part_count; for (; part != end; part++) { - if (key_def_find(first, part->fieldno, part->path, - part->path_len) != NULL) + if (key_def_contains_part(first, part)) continue; if (part->path != NULL) { new_def->parts[pos].path = data; diff --git a/src/box/key_def.h b/src/box/key_def.h index 693dd5dd0..3387cb6be 100644 --- a/src/box/key_def.h +++ b/src/box/key_def.h @@ -339,13 +339,10 @@ key_def_decode_parts_160(struct key_part_def *parts, uint32_t part_count, const char **data, const struct field_def *fields, uint32_t field_count); -/** - * Returns the part in index_def->parts for the specified fieldno. - * If fieldno is not in index_def->parts returns NULL. - */ -const struct key_part * -key_def_find(const struct key_def *key_def, uint32_t fieldno, const char *path, - uint32_t path_len); +/** Check if @a key_def contains @a to_find part. */ +bool +key_def_contains_part(const struct key_def *key_def, + const struct key_part *to_find); /** * Check if key definition @a first contains all parts of diff --git a/src/box/tuple.c b/src/box/tuple.c index 70eda36b3..83bdad1a3 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -168,12 +168,13 @@ tuple_validate_raw(struct tuple_format *format, const char *tuple) field->is_nullable)) return -1; /* Check all JSON paths. */ - if (field->childs != NULL && - tuple_field_bypass_and_init(field, i, tuple, &pos, - NULL) != 0) - return -1; - if (field->childs == NULL) + if (field->childs != NULL) { + if (tuple_field_bypass_and_init(field, i, tuple, &pos, + NULL) != 0) + return -1; + } else { mp_next(&pos); + } } return 0; } diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 243c81f2c..51624d243 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -295,7 +295,6 @@ tuple_field_bypass_and_init(const struct tuple_field *field, uint32_t idx, 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; @@ -317,9 +316,9 @@ tuple_field_bypass_and_init(const struct tuple_field *field, uint32_t idx, 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); + int 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", @@ -392,10 +391,8 @@ tuple_field_bypass_and_init(const struct tuple_field *field, uint32_t idx, } assert(offset != NULL); if (field_map != NULL && - field->offset_slot != TUPLE_OFFSET_SLOT_NIL) { - field_map[field->offset_slot] = - (uint32_t) (*offset - tuple); - } + field->offset_slot != TUPLE_OFFSET_SLOT_NIL) + field_map[field->offset_slot] = (uint32_t) (*offset - tuple); mp_next(offset); return 0; @@ -404,15 +401,13 @@ error_type_mistmatch: mp_type_strs[type], valid_type_str); error_invalid_document: assert(err != NULL); - do { - char *data_buff = tt_static_buf(); - mp_snprint(data_buff, TT_STATIC_BUF_LEN, mp_data); - const char *err_msg = - tt_sprintf("invalid field %d document content '%s': %s", - idx + TUPLE_INDEX_BASE, data_buff, err); - diag_set(ClientError, ER_DATA_STRUCTURE_MISMATCH, err_msg); - return -1; - } while (0); + char *data_buff = tt_static_buf(); + mp_snprint(data_buff, TT_STATIC_BUF_LEN, mp_data); + const char *err_msg = + tt_sprintf("invalid field %d document content '%s': %s", + idx + TUPLE_INDEX_BASE, data_buff, err); + diag_set(ClientError, ER_DATA_STRUCTURE_MISMATCH, err_msg); + return -1; } /** @@ -460,7 +455,7 @@ tuple_format_add_json_path(struct tuple_format *format, const char *path, path_len, path, field_type_strs[field->type]); diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, - node.num, err); + node.num, err); return -1; } if (field->is_nullable != is_nullable) { @@ -568,7 +563,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, if (part->path != NULL) { field->is_key_part = true; - assert(is_sequential == false); + assert(! is_sequential); struct tuple_field *leaf = NULL; if (tuple_format_add_json_path(format, part->path, @@ -593,12 +588,10 @@ 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->path == NULL) { + part->type)) { field->type = part->type; } else if (! field_type1_contains_type2(part->type, - field->type) && - part->path == NULL) { + field->type)) { const char *name; int fieldno = part->fieldno + TUPLE_INDEX_BASE; if (part->fieldno >= field_count) { @@ -626,10 +619,8 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, * so we don't store an offset for it. */ if (field->offset_slot == TUPLE_OFFSET_SLOT_NIL && - is_sequential == false && part->fieldno > 0) { - + !is_sequential && part->fieldno > 0) field->offset_slot = --current_slot; - } } } @@ -708,10 +699,8 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_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; - } + part->path_len, NULL) != 0) + goto error; index_field_count = MAX(index_field_count, part->fieldno + 1); } @@ -742,14 +731,14 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count, if (format == NULL) { diag_set(OutOfMemory, sizeof(struct tuple_format), "malloc", "tuple format"); - return NULL; + goto error; } if (dict == NULL) { assert(space_field_count == 0); format->dict = tuple_dictionary_new(NULL, 0); if (format->dict == NULL) { free(format); - return NULL; + goto error; } } else { format->dict = dict; @@ -768,6 +757,9 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count, format->min_field_count = 0; format->path_hash = path_hash; return format; +error: + json_path_hash_delete(path_hash); + return NULL; } /** Free tuple format resources, doesn't unregister. */ @@ -945,10 +937,6 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map, return -1; } - /* - * First field is simply accessible, store offset to it - * only for JSON path. - */ uint32_t i = 0; enum mp_type mp_type; const struct tuple_field *field = &format->fields[0]; @@ -961,6 +949,10 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map, format->field_map_size); } if (field->childs == NULL) { + /* + * First field is simply accessible, do not store + * offset to it. + */ mp_type = mp_typeof(*pos); if (key_mp_type_validate(field->type, mp_type, ER_FIELD_TYPE, TUPLE_INDEX_BASE, field->is_nullable))
next prev parent reply other threads:[~2018-09-03 10:35 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-27 7:37 [tarantool-patches] [PATCH v3 0/4] box: indexes by JSON path Kirill Shcherbatov 2018-08-27 7:37 ` [tarantool-patches] [PATCH v3 1/4] rfc: describe a Tarantool JSON indexes Kirill Shcherbatov 2018-08-27 7:37 ` [tarantool-patches] [PATCH v3 2/4] box: introduce slot_cache in key_part Kirill Shcherbatov 2018-09-03 10:35 ` [tarantool-patches] " Vladislav Shpilevoy 2018-09-06 12:47 ` Kirill Shcherbatov 2018-09-17 17:08 ` Vladimir Davydov 2018-08-27 7:37 ` [tarantool-patches] [PATCH v3 3/4] box: introduce JSON indexes Kirill Shcherbatov 2018-09-03 10:32 ` [tarantool-patches] " Vladislav Shpilevoy 2018-09-03 10:35 ` Vladislav Shpilevoy [this message] 2018-09-06 12:46 ` Kirill Shcherbatov 2018-08-27 7:37 ` [tarantool-patches] [PATCH v3 4/4] box: specify indexes in user-friendly form Kirill Shcherbatov 2018-09-03 10:32 ` [tarantool-patches] " Vladislav Shpilevoy 2018-09-06 12:46 ` Kirill Shcherbatov 2018-09-17 15:50 ` [tarantool-patches] [PATCH v3 0/4] box: indexes by JSON path Vladimir Davydov
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=20da5c2d-1eec-f951-807f-00f2e6873bcd@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v3 3/4] box: introduce JSON indexes' \ /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