[tarantool-patches] Re: [PATCH v1 1/1] lib: introduce a new JSON_TOKEN_ANY json token
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Feb 28 20:10:47 MSK 2019
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.
More information about the Tarantool-patches
mailing list