Re: [PATCH v5 1/4] box: introduce tuple_format_iterator class

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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;
> +}