[tarantool-patches] Re: [PATCH v5 2/9] lib: implement JSON tree class for json library
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Dec 5 12:07:40 MSK 2018
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(-)
More information about the Tarantool-patches
mailing list