Re: [tarantool-patches] Re: [PATCH v5 2/9] lib: implement JSON tree class for json library

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

On Wed, Dec 05, 2018 at 11:37:06AM +0300, Kirill Shcherbatov wrote:
> Hi! Thank you for review.
> 
> >>> BTW, json array start indexing from 0, not 1 AFAIK. Starting indexing
> >>> from 1 looks weird to me.
> > 
> > You left this comment from my previous review unattended.
> 
> In fact, it is not so; we use [token.num - 1] to retrieve field.
> Let's better describe it in comment:
> 	/**
> 	 * Array of child records. Indexes in this array
> 	 * match [token.num-1] index for JSON_TOKEN_NUM  type
> 	 * and are allocated sequentially for JSON_TOKEN_STR child
> 	 * tokens.
> 	 */
> 	struct json_token **children;

This is weird: AFAIU json_lexer may return token.num equal to 0.
What happens if we try to insert such a token into a tree? I think 
we should insert a token at children[token.num], not [toekn.num-1].

> 
> > As I've already told you, should be
> > Needed for #1012
> 
> > #ifndef/endif shouldn't be indented.
> 
> > I'd prefer to change this to something simpler, like
> > 
> > 	assert(token->child_count == 0);
> > 
> > but now I realize that child_count isn't actually the number of
> > children, as I thought, but the max id of ever existed child.
> > This is confusing. We need to do something about it.
> > 
> > What about?
> > 
> > 	/**
> > 	 * Allocation size of the children array.
> > 	 */
> > 	int children_capacity;
> > 	/**
> > 	 * Max occupied index in the children array.
> > 	 */
> > 	int max_child_idx;
> > 
> > and update max_child_idx on json_tree_del() as well
> 
> > You pass token** to mh_json_find instead of token*. I haven't noticed
> > that before, but turns out that
> > 
> >> +#define mh_key_t struct json_token **
> > 
> > This looks weird. Why not
> > 
> >  #define mh_key_t struct json_token *
> 
> Ok
> 
> >> +		return entry != NULL ? *entry : NULL;
> > 
> > AFAIU entry can't be NULL here.
> 
> 		assert(entry != NULL);
> 		return *entry;
> 
> >> +#define json_tree_foreach_entry_safe(root, node, type, member, tmp)	     \
> >> +	for ((node) = json_tree_postorder_next_entry((root), NULL,	     \
> >> +						     type, member);	     \
> >> +	     &(node)->member != (root) &&				     \
> >> +	     ((tmp) =  json_tree_postorder_next_entry((root),		     \
> > 
> > Extra space.
> 
> Ok

These 'Ok'-s only clutter the email. If you agree with all my other
comments. You can simply write "Agreed with everything else" and don't
quote my comments.

Also, please don't re-push and re-send the patch until we've agreed on
all points. In this particular case the question about whether we should
start indexing from 0 or 1 remains.

> 
> ===============================================
> 
> New JSON tree class would store JSON paths for tuple fields
> for registered non-plain indexes. It is a hierarchical data
> structure that organize JSON nodes produced by parser.
> Class provides API to lookup node by path and iterate over the
> tree.
> JSON Indexes patch require such functionality to make lookup
> for tuple_fields by path, make initialization of field map and
> build vynyl_stmt msgpack for secondary index via JSON tree
> iteration.
> 
> Needed for #1012
> ---
>  src/lib/json/CMakeLists.txt |   1 +
>  src/lib/json/json.c         | 257 ++++++++++++++++++++++++++++++++
>  src/lib/json/json.h         | 287 +++++++++++++++++++++++++++++++++++-
>  test/unit/json_path.c       | 237 ++++++++++++++++++++++++++++-
>  test/unit/json_path.result  |  60 +++++++-
>  5 files changed, 839 insertions(+), 3 deletions(-)