Re: [PATCH v4 03/14] box: introduce tuple_field_by_relative_path

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

On Thu, Oct 11, 2018 at 10:58:48AM +0300, Kirill Shcherbatov wrote:
> The new tuple_field_by_relative_path routine is used in function
> tuple_field_raw_by_path to retrieve data by JSON path from field.
> We need this routine exported in future to access data by JSON
> path specified in key_part.
> 
> Part of #1012.
> ---
>  src/box/tuple_format.c | 60 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index b385c0d..5679cad 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -541,6 +541,42 @@ tuple_field_go_to_key(const char **field, const char *key, int len)
>  	return -1;
>  }
>  
> +/**
> + * Retrieve field data by JSON path.
> + * @param field Pointer to msgpack with data.
> + * @param path The path to process.
> + * @param path_len The length of the @path.
> + * @retval 0 On success.
> + * @retval >0 On path parsing error, invalid character position.
> + */
> +static inline int
> +tuple_field_by_relative_path(const char **field, const char *path,
> +			     uint32_t path_len)

I kinda dislike the function name. See, we have tuple_field_by_path,
which takes a tuple struct, and tuple_field_by_relative_path, which
takes raw msgpack. Please think of a better name so that everything
looks consistent.

Also, this function doesn't (and won't looking at the further patches)
look up tuple_field/offset_slot, which is another unsettiling difference
from tuple_field_by_path, despite similar names. May be, it should
descend the JSON tree as well?

> +{
> +	int rc;
> +	struct json_path_parser parser;
> +	struct json_path_node node;
> +	json_path_parser_create(&parser, path, path_len);

I don't like that you create a parser even if this function is called
from tuple_field_raw_by_path: you could pass the parser created by the
latter. I mean, you'd have two functions, one takes a relative path,
it's going to be public. Another takes a parser. The latter would be
called by the former and by tuple_field_raw_by_path.

> +	while ((rc = json_path_next(&parser, &node)) == 0) {
> +		switch (node.type) {
> +		case JSON_PATH_NUM:
> +			rc = tuple_field_go_to_index(field, node.num);
> +			break;
> +		case JSON_PATH_STR:
> +			rc = tuple_field_go_to_key(field, node.str, node.len);
> +			break;
> +		default:
> +			assert(node.type == JSON_PATH_END);
> +			return 0;
> +		}
> +		if (rc != 0) {
> +			*field = NULL;
> +			return 0;
> +		}
> +	}
> +	return rc;
> +}