From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 28 Feb 2019 20:10:47 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH v1 1/1] lib: introduce a new JSON_TOKEN_ANY json token Message-ID: <20190228171047.hmemgyzmjjmljcos@esperanza> References: <51b08c09ebe40d52bc3b6b6b63755930040500f6.1550571905.git.kshcherbatov@tarantool.org> <20190226165832.jvwc55ctxj5zhpgt@esperanza> <078abd2e-76d9-373e-07db-e70286faef3f@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <078abd2e-76d9-373e-07db-e70286faef3f@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Wed, Feb 27, 2019 at 05:07:30PM +0300, Kirill Shcherbatov wrote: > diff --git a/src/box/key_def.c b/src/box/key_def.c > index 432b72a97..6e818f20a 100644 > --- a/src/box/key_def.c > +++ b/src/box/key_def.c > @@ -196,12 +196,30 @@ key_def_new(const struct key_part_def *parts, uint32_t part_count) > if (coll_id == NULL) { > diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > i + 1, "collation was not found by ID"); > - key_def_delete(def); > - return NULL; > + goto error; > } > coll = coll_id->coll; > } > - uint32_t path_len = part->path != NULL ? strlen(part->path) : 0; > + uint32_t path_len = 0; > + if (part->path != NULL) { > + path_len = strlen(part->path); > + int rc; > + struct json_lexer lexer; > + struct json_token token; > + json_lexer_create(&lexer, part->path, path_len, > + TUPLE_INDEX_BASE); > + while ((rc = json_lexer_next_token(&lexer, > + &token)) == 0) { > + if (token.type == JSON_TOKEN_ANY) { > + diag_set(ClientError, ER_UNSUPPORTED, > + "Index", "multikey path yet"); ER_UNSUPPORTED, "Tarantool", "multikey indexes". > + goto error; > + } else if (token.type == JSON_TOKEN_END) { > + break; > + } > + } > + assert(rc == 0 && token.type == JSON_TOKEN_END); > + } This is a bit too much for a simple stub forbidding multikey indexes. Let's add JSON_TOKEN_ANY check to tuple_format_add_field() for now. It will be more compact, because we already parse a path there, hence it will be easier to remove it. > key_def_set_part(def, i, part->fieldno, part->type, > part->nullable_action, coll, part->coll_id, > part->sort_order, part->path, path_len, > diff --git a/src/lib/json/json.c b/src/lib/json/json.c > index 1b1a3ec2c..2a28d5d8e 100644 > --- a/src/lib/json/json.c > +++ b/src/lib/json/json.c > @@ -252,10 +256,7 @@ json_lexer_next_token(struct json_lexer *lexer, struct json_token *token) > } > } > > -/** > - * Compare JSON token keys. > - */ > -static int > +int You don't need to export this function any more. Please be more careful and thoroughly self-review your patches before submitting them for review. > json_token_cmp(const struct json_token *a, const struct json_token *b) > { > if (a->parent != b->parent) > @@ -479,6 +486,8 @@ json_tree_add(struct json_tree *tree, struct json_token *parent, > struct json_token *token) > { > assert(json_tree_lookup(tree, parent, token) == NULL); > + assert(token->type != JSON_TOKEN_ANY || > + (!json_token_is_multikey(parent) && json_token_is_leaf(parent))); AFAIU you don't need this assertion - (json_token_lookup == NULL) check should be enough, no? > token->parent = parent; > token->children = NULL; > token->children_capacity = 0; > @@ -532,6 +541,8 @@ json_tree_del(struct json_tree *tree, struct json_token *token) > assert(token->sibling_idx >= 0); > assert(parent->children[token->sibling_idx] == token); > assert(json_tree_lookup(tree, parent, token) == token); > + assert(token->type != JSON_TOKEN_ANY || > + (json_token_is_multikey(parent) && parent->max_child_idx == 0)); Ditto. > /* > * Clear the entry corresponding to this token in parent's > * children array and update max_child_idx if necessary. > diff --git a/src/lib/json/json.h b/src/lib/json/json.h > index 66cddd026..c1c61c447 100644 > --- a/src/lib/json/json.h > +++ b/src/lib/json/json.h > @@ -98,6 +99,8 @@ struct json_token { > * array match [token.num] index for JSON_TOKEN_NUM type > * and are allocated sequentially for JSON_TOKEN_STR child > * tokens. > + * If in case of "multikey entry", children array has the > + * only child JSON_TOKEN_ANY with index 0. Please always add an empty line between paragraphs. Anyway, let's rewrite this appendix to the comment as follows: JSON_TOKEN_ANY is exclusive. If it's present, it must be the only one and have index 0 in the children array. It will be returned by lookup by any key. > */ > struct json_token **children; > /** Allocation size of children array. */ > diff --git a/test/unit/json.c b/test/unit/json.c > index 6448a3210..2e24d60f1 100644 > --- a/test/unit/json.c > +++ b/test/unit/json.c > @@ -411,6 +411,54 @@ test_tree() > "last node became interm - it can't be leaf anymore"); > is(json_token_is_leaf(&records[3].node), true, "last node is leaf"); > > + json_tree_foreach_entry_safe(node, &tree.root, struct test_struct, > + node, node_tmp) > + json_tree_del(&tree, &node->node); > + > + /* Test multikey tokens. */ > + records_idx = 0; > + char *path_problem = "base[3]"; Why 'path_problem'? The variable name is confusing. Let's please simply reuse 'path' variable defined above in the code. > + node = test_add_path(&tree, path_problem, strlen(path_problem), > + records, &records_idx); > + is(node, &records[1], "add path '%s'", path_problem); > + token->type = JSON_TOKEN_ANY; > + node = json_tree_lookup_entry(&tree, &records[0].node, token, > + struct test_struct, node); > + is(node->node.num, 2, "lookup any token in non-multikey node"); > + > + /* Can't attay ANY token to non-leaf parent. Cleanup. */ s/attay/attach s/parent/node > + json_tree_del(&tree, &records[1].node); > + records_idx--; > + > + char *path_multikey = "base[*][\"data\"]"; Again, let's please reuse 'path' and 'path_len' defined in the beginning of this function rather than adding a new local variable. > + node = test_add_path(&tree, path_multikey, strlen(path_multikey), > + records, &records_idx); > + is(node, &records[2], "add path '%s'", path_multikey); > + > + node = json_tree_lookup_path_entry(&tree, &tree.root, path_multikey, > + strlen(path_multikey), INDEX_BASE, > + struct test_struct, node); > + is(node, &records[2], "lookup path '%s'", path_multikey); > + > + token = &records[records_idx++].node; > + token->type = JSON_TOKEN_NUM; > + token->num = 3; > + node = json_tree_lookup_entry(&tree, &records[0].node, token, > + struct test_struct, node); > + is(node, &records[1], "lookup numeric token in multikey node"); > + > + token->type = JSON_TOKEN_ANY; > + node = json_tree_lookup_entry(&tree, &records[0].node, token, > + struct test_struct, node); > + is(node, &records[1], "lookup any token in multikey node"); > + > + token->type = JSON_TOKEN_STR; > + token->str = "str"; > + token->len = strlen("str"); > + node = json_tree_lookup_entry(&tree, &records[0].node, token, > + struct test_struct, node); > + is(node, &records[1], "lookup string token in multikey node"); > + > json_tree_foreach_entry_safe(node, &tree.root, struct test_struct, > node, node_tmp) > json_tree_del(&tree, &node->node); > diff --git a/test/unit/json.result b/test/unit/json.result > index ee54cbe0e..02080f93a 100644 > --- a/test/unit/json.result > +++ b/test/unit/json.result > @@ -160,10 +160,17 @@ ok 2 - subtests > ok 56 - last node is leaf > ok 57 - last node became interm - it can't be leaf anymore > ok 58 - last node is leaf > + ok 59 - add path 'base[3]' > + ok 60 - lookup any token in non-multikey node > + ok 61 - add path 'base[*]["data"]' > + ok 62 - lookup path 'base[*]["data"]' > + ok 63 - lookup numeric token in multikey node > + ok 64 - lookup any token in multikey node > + ok 65 - lookup string token in multikey node > ok 3 - subtests > *** test_tree: done *** > *** test_path_cmp *** > - 1..7 > + 1..8 > ok 1 - path cmp result "Data[1]["FIO"].fname" with "Data[1]["FIO"].fname": have 0, expected 0 > ok 2 - path cmp result "Data[1]["FIO"].fname" with "["Data"][1].FIO["fname"]": have 0, expected 0 > ok 3 - path cmp result "Data[1]["FIO"].fname" with "Data[1]": have 1, expected 1 > @@ -171,6 +178,7 @@ ok 3 - subtests > ok 5 - path cmp result "Data[1]["FIO"].fname" with "Data[1]["Info"].fname[1]": have -1, expected -1 > ok 6 - path Data[1]["FIO"].fname is valid > ok 7 - path Data[[1]["FIO"].fname error pos 6 expected 6 > + ok 8 - path cmp result "Data[*]["FIO"].fname[*]" with "["Data"][*].FIO["fname"][*]": have 0, expected 0 Nit: Let's please move this test case a bit above so that all 'path cmp' are gathered together.