From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 5 Dec 2018 12:07:40 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v5 2/9] lib: implement JSON tree class for json library Message-ID: <20181205090740.lyt6ikf7wmivavqb@esperanza> References: <02671a3d0a2236ecd6e12c0bc51b7f5e39272a2f.1543229303.git.kshcherbatov@tarantool.org> <20181129173816.kprfjhki5o7ytfbl@esperanza> <3c7bb503-561c-19b0-1197-f714b6f384d4@tarantool.org> <20181204175412.dayx2wplbxi5rrfz@esperanza> <38c62fa1-190d-181f-621b-8185847055f7@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <38c62fa1-190d-181f-621b-8185847055f7@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, Kostya Osipov List-ID: 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(-)