From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 6 May 2019 16:55:55 +0300 From: Vladimir Davydov Subject: Re: [PATCH v5 1/4] box: introduce tuple_format_iterator class Message-ID: <20190506135555.wgtewxgznfta3uss@esperanza> References: <0e9e576a96eadd8bf20de1ffe3635ac543d9de9c.1557142159.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0e9e576a96eadd8bf20de1ffe3635ac543d9de9c.1557142159.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Mon, May 06, 2019 at 02:57:38PM +0300, Kirill Shcherbatov wrote: > The similar code in tuple_field_map_create and > vy_stmt_new_surrogate_delete_raw that performs tuple decode > with tuple_format has been refactored as reusable > tuple_format_iterator class. > > Being thus encapsulated, this code will be uniformly managed and > extended in the further patches in scope of multikey indexes. > > Extended engine/json test with vy_stmt_new_surrogate_delete_raw > corner case test. There was no problem before this patch, but > small bug appeared during tuple_format_iterator_next > implementation was not covered. > > Needed for #1257 > --- > src/box/tuple_format.c | 356 ++++++++++++++++++++------------------ > src/box/tuple_format.h | 118 +++++++++++++ > src/box/vy_stmt.c | 117 +++++-------- > test/engine/json.result | 17 ++ > test/engine/json.test.lua | 5 + > 5 files changed, 371 insertions(+), 242 deletions(-) Looks good. See a few really minor nit picks below. > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 804a678a1..de48e9bb0 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -1033,3 +883,179 @@ box_tuple_format_unref(box_tuple_format_t *format) > tuple_format_unref(format); > } > > +int > +tuple_format_iterator_create(struct tuple_format_iterator *it, > + struct tuple_format *format, const char *tuple, > + uint8_t flags, uint32_t *defined_field_count, > + struct region *region) > +{ > + assert(mp_typeof(*tuple) == MP_ARRAY); > + *defined_field_count = mp_decode_array(&tuple); > + bool validate = flags & TUPLE_FORMAT_ITERATOR_VALIDATE; > + if (validate && format->exact_field_count > 0 && > + format->exact_field_count != *defined_field_count) { > + diag_set(ClientError, ER_EXACT_FIELD_COUNT, > + (unsigned) *defined_field_count, > + (unsigned) format->exact_field_count); > + return -1; > + } > + it->parent = &format->fields.root; > + it->format = format; > + it->pos = tuple; > + it->flags = flags; > + it->required_fields = NULL; > + it->required_fields_sz = 0; > + > + uint32_t frames_sz = format->fields_depth * sizeof(struct mp_frame); > + if (validate) > + it->required_fields_sz = bitmap_size(format->total_field_count); > + uint32_t total_sz = frames_sz + 2 * it->required_fields_sz; 2 * ? Shouldn't this be a part of the next patch? > + struct mp_frame *frames = region_alloc(region, total_sz); > + if (frames == NULL) { > + diag_set(OutOfMemory, total_sz, "region", > + "tuple_format_iterator"); > + return -1; > + } > + mp_stack_create(&it->stack, format->fields_depth, frames); > + *defined_field_count = MIN(*defined_field_count, validate ? Shouldn't we use KEY_PARTS_ONLY here rather than VALIDATE? > + tuple_format_field_count(format) : > + format->index_field_count); > + mp_stack_push(&it->stack, MP_ARRAY, *defined_field_count); > + > + if (validate) { > + it->required_fields = (char *)frames + frames_sz; > + memcpy(it->required_fields, format->required_fields, > + it->required_fields_sz); > + } > + return 0; > +} > + > +/** > + * Scan required_fields bitmap and raise error when it is > + * non-empty. > + * @sa format:required_fields and field:multikey_required_fields > + * definition. There's no such thing as multikey_required_fields yet. > + */ > +static int > +tuple_format_required_fields_validate(struct tuple_format *format, > + void *required_fields, > + uint32_t required_fields_sz) > +{ > + struct bit_iterator it; > + bit_iterator_init(&it, required_fields, required_fields_sz, true); > + size_t id = bit_iterator_next(&it); > + if (id < SIZE_MAX) { > + /* A field is missing, report an error. */ > + struct tuple_field *field = > + tuple_format_field_by_id(format, id); > + assert(field != NULL); > + diag_set(ClientError, ER_FIELD_MISSING, > + tuple_field_path(field)); > + return -1; > + } > + return 0; > +} > + > +int > +tuple_format_iterator_next(struct tuple_format_iterator *it, > + struct tuple_format_iterator_entry *entry) > +{ > + entry->data = it->pos; > + struct mp_frame *frame = mp_stack_top(&it->stack); > + while (!mp_frame_advance(frame)) { > + /* > + * If the elements of the current frame > + * are over, pop this frame out of stack > + * and climb one position in the format::fields > + * tree to match the changed JSON path to the > + * data in the tuple. > + */ > + mp_stack_pop(&it->stack); > + if (mp_stack_is_empty(&it->stack)) > + goto eof; > + frame = mp_stack_top(&it->stack); > + it->parent = it->parent->parent; > + } > + entry->parent = > + it->parent != &it->format->fields.root ? > + json_tree_entry(it->parent, struct tuple_field, token) : NULL; > + /* > + * Use the top frame of the stack and the > + * current data offset to prepare the JSON token > + * and perform subsequent format::fields lookup. > + */ > + struct json_token token; > + switch (frame->type) { > + case MP_ARRAY: > + token.type = JSON_TOKEN_NUM; > + token.num = frame->idx; > + break; > + case MP_MAP: > + if (mp_typeof(*it->pos) != MP_STR) { > + entry->data = it->pos; > + entry->field = NULL; > + mp_next(&it->pos); > + mp_next(&it->pos); What about entry->data_end? > + return 0; > + } > + token.type = JSON_TOKEN_STR; > + token.str = mp_decode_str(&it->pos, (uint32_t *)&token.len); > + break; > + default: > + unreachable(); > + } > + assert(it->parent != NULL); > + struct tuple_field *field = > + json_tree_lookup_entry(&it->format->fields, it->parent, &token, > + struct tuple_field, token); > + if (it->flags & TUPLE_FORMAT_ITERATOR_KEY_PARTS_ONLY && > + field != NULL && !field->is_key_part) > + field = NULL; > + entry->field = field; > + entry->data = it->pos; > + /* > + * If the current position of the data in tuple > + * matches the container type (MP_MAP or MP_ARRAY) > + * and the format::fields tree has such a record, > + * prepare a new stack frame because it needs to > + * be analyzed in the next iterations. > + */ > + enum mp_type type = mp_typeof(*it->pos); > + if ((type == MP_ARRAY || type == MP_MAP) && > + !mp_stack_is_full(&it->stack) && field != NULL) { > + uint32_t size = type == MP_ARRAY ? > + mp_decode_array(&it->pos) : > + mp_decode_map(&it->pos); > + entry->count = size; > + mp_stack_push(&it->stack, type, size); > + it->parent = &field->token; > + } else { > + entry->count = 0; > + mp_next(&it->pos); > + } > + entry->data_end = it->pos; > + if (field == NULL || (it->flags & TUPLE_FORMAT_ITERATOR_VALIDATE) == 0) > + return 0; > + > + /* > + * Check if field mp_type is compatible with type > + * defined in format. > + */ > + bool is_nullable = tuple_field_is_nullable(field); > + if (!field_mp_type_is_compatible(field->type, mp_typeof(*entry->data), > + is_nullable) != 0) { > + diag_set(ClientError, ER_FIELD_TYPE, > + tuple_field_path(field), > + field_type_strs[field->type]); > + return -1; > + } > + bit_clear(it->required_fields, field->id); > + return 0; > +eof: > + if (it->flags & TUPLE_FORMAT_ITERATOR_VALIDATE && > + tuple_format_required_fields_validate(it->format, > + it->required_fields, it->required_fields_sz) != 0) > + return -1; > + entry->data = NULL; > + return 0; > +}