From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 56327286BE for ; Mon, 3 Sep 2018 06:35:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2o6Vqd030cKm for ; Mon, 3 Sep 2018 06:35:59 -0400 (EDT) Received: from smtp2.mail.ru (smtp2.mail.ru [94.100.179.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E247D28428 for ; Mon, 3 Sep 2018 06:35:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 3/4] box: introduce JSON indexes From: Vladislav Shpilevoy References: <8f6030b28e435748f3496d88a4d727c998dc413f.1535354849.git.kshcherbatov@tarantool.org> Message-ID: <20da5c2d-1eec-f951-807f-00f2e6873bcd@tarantool.org> Date: Mon, 3 Sep 2018 13:35:52 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Forgot to send diff: commit deedcb31040cfbe5b93393e54ec55f3cde5631b8 Author: Vladislav Shpilevoy 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))