[PATCH v3 5/7] box: introduce tuple_parse_iterator class
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Apr 3 17:04:40 MSK 2019
On Tue, Apr 02, 2019 at 06:49:36PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
> index 22a0fb232..bef1d0903 100644
> --- a/src/box/tuple_format.h
> +++ b/src/box/tuple_format.h
> @@ -412,6 +412,71 @@ tuple_field_map_create(struct tuple_format *format, const char *tuple,
> int
> tuple_format_init();
>
> +/**
> + * A tuple msgpack iterator that parse tuple in deep an returns
'in deep' is an idiom, which isn't appropriate in your case.
Please rephrase.
> + * only fields that are described in the tuple_format.
This isn't quite correct. Suppose I have field [1][3] indexed. Then this
iterator will return [1][1] and [1][2] in addition to [1][3] even if
[1][1] and [1][2] are not described in the format.
> + */
> +struct tuple_parse_iterator {
> + /**
> + * Tuple format is used to perform field lookups in
> + * format::fields JSON tree.
> + */
> + struct tuple_format *format;
> + /**
> + * The pointer to the parent node in the format::fields
> + * JSON tree. Is required for relative lookup for the
> + * next field.
> + */
> + struct json_token *parent;
> + /**
> + * Traversal stack of msgpack frames is used to determine
> + * when the parsing of the current composite mp structure
> + * (array or map) is completed to update to the parent
> + * pointer accordingly.
> + */
> + struct mp_stack stack;
> + /** The current read position in msgpack. */
> + const char *pos;
> +};
> +
> +/**
> + * Initialize tuple parse iterator with tuple format, data pointer
> + * and the count of top-level msgpack fields to be processed.
> + *
> + * This function assumes that the msgpack header containing the
> + * number of top-level msgpack fields (field_count) has already
> + * been parsed and the data pointer has already been shifted
> + * correspondingly. This allows directly limit the number of
> + * fields that must be parsed.
I would try to hide the first mp_decode_array inside the iterator, too,
if possible. Otherwise we have tuple decoding split between two
independent functions.
> +
> + * Function uses the region for the traversal stack allocation.
> + *
> + * Returns 0 on success. In case of memory allocation error sets
> + * diag message and returns -1.
> + */
> +int
> +tuple_parse_iterator_create(struct tuple_parse_iterator *it,
> + struct tuple_format *format, const char *data,
> + uint32_t field_count, struct region *region);
> +
> +/**
> + * Parse tuple in deep and update iterator state.
> + *
> + * Returns the number of fields at the current tuple nesting
> + * level that have been processed (2 for map item, 1 for array
> + * key:value pair, 0 on stop) and initializes:
Using the function return value to inform the caller of whether a field
is a map or an array in such an indirect way looks dubious. May be, use
a separate argument to return the type of the currently decoded
container instead (MP_MAP or MP_ARRAY)?
> + * field - the tuple_field pointer to format::fields field
> + * that matches to the currently processed msgpack field
> + * (when exists),
> + * data - the pointer to the currently processed msgpack field,
> + * data_end - the pointer to the end of currently processed
> + * msgpack field.
In case of indexed array/map field data_end points not to the end of the
current field, but to the end of array/map header. It works fine,
because data_end isn't used by the caller. Still, it looks suspicious.
> + */
> +int
> +tuple_parse_iterator_advice(struct tuple_parse_iterator *it,
> + struct tuple_field **field, const char **data,
> + const char **data_end);
> +
Not sure about the name: strictly speaking, this function doesn't parse
a tuple - it decodes it.
Also, we usually call functions subject_verb, not subject_noun.
More information about the Tarantool-patches
mailing list