Re: [tarantool-patches] Re: [PATCH v1 1/1] lib: introduce a new JSON_TOKEN_ANY json token

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

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.