From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Kirill Shcherbatov Subject: Re: [tarantool-patches] Re: [PATCH v5 3/9] box: manage format fields with JSON tree class References: <0ff5601b664e42d70d71be658b4ba45fe1237564.1543229303.git.kshcherbatov@tarantool.org> <20181129190757.oyu7kom6drgacd3j@esperanza> Message-ID: <5133fc03-e2ed-0bea-d3d1-47b4b60121e9@tarantool.org> Date: Tue, 4 Dec 2018 18:47:16 +0300 MIME-Version: 1.0 In-Reply-To: <20181129190757.oyu7kom6drgacd3j@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov Cc: Kostya Osipov List-ID: > We usually call methods that allocate/free an object _new/delete, not > _create/destroy. Please fix. Ok, done > I've looked at your following patches - this is the only place where you > pass a non-NULL token to tuple_field_create. Let's not pass token to > this function at all, and initialize field->token directly here. Ok, done > diag_set missing Ok, done > And you have exactly the same piece of code in tuple_format_destroy. > Factor it out to a helper function? Ok, static inline void tuple_format_field_tree_destroy(struct tuple_format *format) > /** Link in tuple_format::fields. */ > > would be more useful. Ok, done >> + /** JSON tree of fields. */ > > Would be prudent to say a few words about the tree structure, e.g. > > /** > * Fields comprising the format, organized in a tree. > * First level nodes correspond to tuple fields. > * Deeper levels define indexed JSON paths within > * tuple fields. Nodes of the tree are linked by > * tuple_field::token. > */ Ok, done > format->tree - what tree? > > Why not leave the same name - 'fields'? Ok, done > Comments, comments... /** * Return a count of first level nodes correspond to tuple * fields. */ static inline uint32_t tuple_format_field_count(const struct tuple_format *format) /** * Get the first level node corresponding to tuple field by its * fieldno. */ static inline struct tuple_field * tuple_format_field(struct tuple_format *format, uint32_t fieldno) ========================================= As we going to work with format fields in a unified way, we started to use the tree JSON class for working with first-level format fields. Need for #1012 --- src/box/sql.c | 16 +++--- src/box/sql/build.c | 5 +- src/box/tuple.c | 10 ++-- src/box/tuple_format.c | 125 +++++++++++++++++++++++++++++------------ src/box/tuple_format.h | 49 +++++++++++++--- src/box/vy_stmt.c | 4 +- 6 files changed, 150 insertions(+), 59 deletions(-) diff --git a/src/box/sql.c b/src/box/sql.c index 7b41c9926..9effe1eb2 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -201,7 +201,8 @@ tarantoolSqlite3TupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size) struct tuple_format *format = tuple_format(pCur->last_tuple); assert(format->exact_field_count == 0 || fieldno < format->exact_field_count); - if (format->fields[fieldno].offset_slot == TUPLE_OFFSET_SLOT_NIL) + if (tuple_format_field(format, fieldno)->offset_slot == + TUPLE_OFFSET_SLOT_NIL) return NULL; const char *field = tuple_field(pCur->last_tuple, fieldno); const char *end = field; @@ -896,7 +897,7 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, struct key_def *key_def; const struct tuple *tuple; const char *base; - const struct tuple_format *format; + struct tuple_format *format; const uint32_t *field_map; uint32_t field_count, next_fieldno = 0; const char *p, *field0; @@ -914,7 +915,7 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, base = tuple_data(tuple); format = tuple_format(tuple); field_map = tuple_field_map(tuple); - field_count = format->field_count; + field_count = tuple_format_field_count(format); field0 = base; mp_decode_array(&field0); p = field0; for (i = 0; i < n; i++) { /* @@ -932,9 +933,10 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, uint32_t fieldno = key_def->parts[i].fieldno; if (fieldno != next_fieldno) { + struct tuple_field *field = + tuple_format_field(format, fieldno); if (fieldno >= field_count || - format->fields[fieldno].offset_slot == - TUPLE_OFFSET_SLOT_NIL) { + field->offset_slot == TUPLE_OFFSET_SLOT_NIL) { /* Outdated field_map. */ uint32_t j = 0; @@ -942,9 +944,7 @@ tarantoolSqlite3IdxKeyCompare(struct BtCursor *cursor, while (j++ != fieldno) mp_next(&p); } else { - p = base + field_map[ - format->fields[fieldno].offset_slot - ]; + p = base + field_map[field->offset_slot]; } } next_fieldno = fieldno + 1; diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 52f0bde15..b5abaeeda 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -936,8 +936,9 @@ sql_column_collation(struct space_def *def, uint32_t column, uint32_t *coll_id) struct coll_id *collation = coll_by_id(*coll_id); return collation != NULL ? collation->coll : NULL; } - *coll_id = space->format->fields[column].coll_id; - return space->format->fields[column].coll; + struct tuple_field *field = tuple_format_field(space->format, column); + *coll_id = field->coll_id; + return field->coll; } struct ExprList * diff --git a/src/box/tuple.c b/src/box/tuple.c index ef4d16f39..aae1c3cdd 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -138,7 +138,7 @@ runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple) int tuple_validate_raw(struct tuple_format *format, const char *tuple) { - if (format->field_count == 0) + if (tuple_format_field_count(format) == 0) return 0; /* Nothing to check */ /* Check to see if the tuple has a sufficient number of fields. */ @@ -158,10 +158,12 @@ tuple_validate_raw(struct tuple_format *format, const char *tuple) } /* Check field types */ - struct tuple_field *field = &format->fields[0]; + struct tuple_field *field = tuple_format_field(format, 0); uint32_t i = 0; - uint32_t defined_field_count = MIN(field_count, format->field_count); - for (; i < defined_field_count; ++i, ++field) { + uint32_t defined_field_count = + MIN(field_count, tuple_format_field_count(format)); + for (; i < defined_field_count; ++i) { + field = tuple_format_field(format, i); if (key_mp_type_validate(field->type, mp_typeof(*tuple), ER_FIELD_TYPE, i + TUPLE_INDEX_BASE, tuple_field_is_nullable(field))) diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 661cfdc94..b801a0eb4 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -38,10 +38,27 @@ 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, - ON_CONFLICT_ACTION_NONE, NULL, COLL_NONE, -}; +static struct tuple_field * +tuple_field_new(void) +{ + struct tuple_field *ret = calloc(1, sizeof(struct tuple_field)); + if (ret == NULL) { + diag_set(OutOfMemory, sizeof(struct tuple_field), "malloc", + "ret"); + return NULL; + } + ret->type = FIELD_TYPE_ANY; + ret->offset_slot = TUPLE_OFFSET_SLOT_NIL; + ret->coll_id = COLL_NONE; + ret->nullable_action = ON_CONFLICT_ACTION_NONE; + return ret; +} + +static void +tuple_field_delete(struct tuple_field *field) +{ + free(field); +} static int tuple_format_use_key_part(struct tuple_format *format, @@ -49,8 +66,8 @@ tuple_format_use_key_part(struct tuple_format *format, const struct key_part *part, bool is_sequential, int *current_slot) { - assert(part->fieldno < format->field_count); - struct tuple_field *field = &format->fields[part->fieldno]; + assert(part->fieldno < tuple_format_field_count(format)); + struct tuple_field *field = tuple_format_field(format, part->fieldno); /* * If a field is not present in the space format, * inherit nullable action of the first key part @@ -138,16 +155,15 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, format->min_field_count = tuple_format_min_field_count(keys, key_count, fields, field_count); - if (format->field_count == 0) { + if (tuple_format_field_count(format) == 0) { format->field_map_size = 0; return 0; } /* Initialize defined fields */ for (uint32_t i = 0; i < field_count; ++i) { - format->fields[i].is_key_part = false; - format->fields[i].type = fields[i].type; - format->fields[i].offset_slot = TUPLE_OFFSET_SLOT_NIL; - format->fields[i].nullable_action = fields[i].nullable_action; + struct tuple_field *field = tuple_format_field(format, i); + field->type = fields[i].type; + field->nullable_action = fields[i].nullable_action; struct coll *coll = NULL; uint32_t cid = fields[i].coll_id; if (cid != COLL_NONE) { @@ -159,12 +175,9 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, } coll = coll_id->coll; } - format->fields[i].coll = coll; - format->fields[i].coll_id = cid; + field->coll = coll; + field->coll_id = cid; } - /* Initialize remaining fields */ - for (uint32_t i = field_count; i < format->field_count; i++) - format->fields[i] = tuple_field_default; int current_slot = 0; @@ -184,7 +197,8 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, } } - assert(format->fields[0].offset_slot == TUPLE_OFFSET_SLOT_NIL); + assert(tuple_format_field(format, 0)->offset_slot == + TUPLE_OFFSET_SLOT_NIL); size_t field_map_size = -current_slot * sizeof(uint32_t); if (field_map_size > UINT16_MAX) { /** tuple->data_offset is 16 bits */ @@ -242,6 +256,19 @@ tuple_format_deregister(struct tuple_format *format) format->id = FORMAT_ID_NIL; } +/** Destroy field JSON tree and release allocated memory. */ +static inline void +tuple_format_field_tree_destroy(struct tuple_format *format) +{ + struct tuple_field *field, *tmp; + json_tree_foreach_entry_safe(&format->fields.root, field, + struct tuple_field, token, tmp) { + json_tree_del(&format->fields, &field->token); + tuple_field_delete(field); + } + json_tree_destroy(&format->fields); +} + static struct tuple_format * tuple_format_alloc(struct key_def * const *keys, uint16_t key_count, uint32_t space_field_count, struct tuple_dictionary *dict) @@ -258,39 +285,60 @@ tuple_format_alloc(struct key_def * const *keys, uint16_t key_count, } } uint32_t field_count = MAX(space_field_count, index_field_count); - uint32_t total = sizeof(struct tuple_format) + - field_count * sizeof(struct tuple_field); - struct tuple_format *format = (struct tuple_format *) malloc(total); + struct tuple_format *format = malloc(sizeof(struct tuple_format)); if (format == NULL) { diag_set(OutOfMemory, sizeof(struct tuple_format), "malloc", "tuple format"); return NULL; } + if (json_tree_create(&format->fields) != 0) { + free(format); + return NULL; + } + struct json_token token; + memset(&token, 0, sizeof(token)); + token.type = JSON_TOKEN_NUM; + for (token.num = TUPLE_INDEX_BASE; + token.num < field_count + TUPLE_INDEX_BASE; token.num++) { + struct tuple_field *field = tuple_field_new(); + if (field == NULL) + goto error; + field->token = token; + if (json_tree_add(&format->fields, &format->fields.root, + &field->token) != 0) { + diag_set(OutOfMemory, 0, "json_tree_add", + "&format->tree"); + tuple_field_delete(field); + 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; - } + if (format->dict == NULL) + goto error; } else { format->dict = dict; tuple_dictionary_ref(dict); } format->refs = 0; format->id = FORMAT_ID_NIL; - format->field_count = field_count; format->index_field_count = index_field_count; format->exact_field_count = 0; format->min_field_count = 0; return format; +error:; + tuple_format_field_tree_destroy(format); + free(format); + return NULL; } /** Free tuple format resources, doesn't unregister. */ static inline void tuple_format_destroy(struct tuple_format *format) { + tuple_format_field_tree_destroy(format); tuple_dictionary_unref(format->dict); } @@ -328,18 +376,21 @@ tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, } bool -tuple_format1_can_store_format2_tuples(const struct tuple_format *format1, - const struct tuple_format *format2) +tuple_format1_can_store_format2_tuples(struct tuple_format *format1, + struct tuple_format *format2) { if (format1->exact_field_count != format2->exact_field_count) return false; - for (uint32_t i = 0; i < format1->field_count; ++i) { - const struct tuple_field *field1 = &format1->fields[i]; + uint32_t format1_field_count = tuple_format_field_count(format1); + uint32_t format2_field_count = tuple_format_field_count(format2); + for (uint32_t i = 0; i < format1_field_count; ++i) { + const struct tuple_field *field1 = + tuple_format_field(format1, i); /* * The field has a data type in format1, but has * no data type in format2. */ - if (i >= format2->field_count) { + if (i >= format2_field_count) { /* * The field can get a name added * for it, and this doesn't require a data @@ -355,7 +406,8 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1, else return false; } - const struct tuple_field *field2 = &format2->fields[i]; + const struct tuple_field *field2 = + tuple_format_field(format2, i); if (! field_type1_contains_type2(field1->type, field2->type)) return false; /* @@ -374,7 +426,7 @@ int tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map, const char *tuple, bool validate) { - if (format->field_count == 0) + if (tuple_format_field_count(format) == 0) return 0; /* Nothing to initialize */ const char *pos = tuple; @@ -397,17 +449,17 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map, /* first field is simply accessible, so we do not store offset to it */ enum mp_type mp_type = mp_typeof(*pos); - const struct tuple_field *field = &format->fields[0]; + const struct tuple_field *field = + tuple_format_field((struct tuple_format *)format, 0); if (validate && key_mp_type_validate(field->type, mp_type, ER_FIELD_TYPE, TUPLE_INDEX_BASE, tuple_field_is_nullable(field))) return -1; mp_next(&pos); /* other fields...*/ - ++field; uint32_t i = 1; uint32_t defined_field_count = MIN(field_count, validate ? - format->field_count : + tuple_format_field_count(format) : format->index_field_count); if (field_count < format->index_field_count) { /* @@ -417,7 +469,8 @@ tuple_init_field_map(const struct tuple_format *format, uint32_t *field_map, memset((char *)field_map - format->field_map_size, 0, format->field_map_size); } - for (; i < defined_field_count; ++i, ++field) { + for (; i < defined_field_count; ++i) { + field = tuple_format_field((struct tuple_format *)format, i); mp_type = mp_typeof(*pos); if (validate && key_mp_type_validate(field->type, mp_type, ER_FIELD_TYPE, diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h index 232df22b2..623f4e25f 100644 --- a/src/box/tuple_format.h +++ b/src/box/tuple_format.h @@ -34,6 +34,7 @@ #include "key_def.h" #include "field_def.h" #include "errinj.h" +#include "json/json.h" #include "tuple_dictionary.h" #if defined(__cplusplus) @@ -113,6 +114,8 @@ struct tuple_field { struct coll *coll; /** Collation identifier. */ uint32_t coll_id; + /** Link in tuple_format::fields. */ + struct json_token token; }; /** @@ -166,16 +169,46 @@ struct tuple_format { * index_field_count <= min_field_count <= field_count. */ uint32_t min_field_count; - /* Length of 'fields' array. */ - uint32_t field_count; /** * Shared names storage used by all formats of a space. */ struct tuple_dictionary *dict; - /* Formats of the fields */ - struct tuple_field fields[0]; + /** + * Fields comprising the format, organized in a tree. + * First level nodes correspond to tuple fields. + * Deeper levels define indexed JSON paths within + * tuple fields. Nodes of the tree are linked by + * tuple_field::token. + */ + struct json_tree fields; }; +/** + * Return a count of first level nodes correspond to tuple + * fields. + */ +static inline uint32_t +tuple_format_field_count(const struct tuple_format *format) +{ + return format->fields.root.child_count; +} + +/** + * Get the first level node corresponding to tuple field by its + * fieldno. + */ +static inline struct tuple_field * +tuple_format_field(struct tuple_format *format, uint32_t fieldno) +{ + assert(fieldno < tuple_format_field_count(format)); + struct json_token token = { + .type = JSON_TOKEN_NUM, + .num = fieldno + TUPLE_INDEX_BASE + }; + return json_tree_lookup_entry(&format->fields, &format->fields.root, + &token, struct tuple_field, token); +} + extern struct tuple_format **tuple_formats; static inline uint32_t @@ -238,8 +271,8 @@ tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, * @retval True, if @a format1 can store any tuples of @a format2. */ bool -tuple_format1_can_store_format2_tuples(const struct tuple_format *format1, - const struct tuple_format *format2); +tuple_format1_can_store_format2_tuples(struct tuple_format *format1, + struct tuple_format *format2); /** * Calculate minimal field count of tuples with specified keys and @@ -333,7 +366,9 @@ tuple_field_raw(const struct tuple_format *format, const char *tuple, return tuple; } - int32_t offset_slot = format->fields[field_no].offset_slot; + int32_t offset_slot = + tuple_format_field((struct tuple_format *)format, + field_no)->offset_slot; if (offset_slot != TUPLE_OFFSET_SLOT_NIL) { if (field_map[offset_slot] != 0) return tuple + field_map[offset_slot]; diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c index d83840406..3e60fece9 100644 --- a/src/box/vy_stmt.c +++ b/src/box/vy_stmt.c @@ -411,7 +411,7 @@ vy_stmt_new_surrogate_from_key(const char *key, enum iproto_type type, uint32_t *field_map = (uint32_t *) raw; char *wpos = mp_encode_array(raw, field_count); for (uint32_t i = 0; i < field_count; ++i) { - const struct tuple_field *field = &format->fields[i]; + const struct tuple_field *field = tuple_format_field(format, i); if (field->offset_slot != TUPLE_OFFSET_SLOT_NIL) field_map[field->offset_slot] = wpos - raw; if (iov[i].iov_base == NULL) { @@ -465,7 +465,7 @@ vy_stmt_new_surrogate_delete_raw(struct tuple_format *format, } char *pos = mp_encode_array(data, field_count); for (uint32_t i = 0; i < field_count; ++i) { - const struct tuple_field *field = &format->fields[i]; + const struct tuple_field *field = tuple_format_field(format, i); if (! field->is_key_part) { /* Unindexed field - write NIL. */ assert(i < src_count); -- 2.19.2