Re: [tarantool-patches] [PATCH v2 2/5] lib: introduce is_weak_cmp flag for json_path_cmp

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/03/19 16:15]:
> Introduced is_weak_cmp flag for json_path_cmp routine: when this
> flag is set, strings are equal when whole b path is an a subpath
> (in terms of tokens).

Could you please provide an example in the changeset comment and
function comment? 
Why did you choose "weak" as a description of this type of
comparison? Perhaps "is_prefix"? cmp is symmetrical, is the new
function symmetrical?

>   * Compare two JSON paths using Lexer class.
>   * - in case of paths that have same token-sequence prefix,
>   *   the path having more tokens is assumed to be greater
> + *   (except the case when is_weak_cmp flag is set).
>   * - both paths must be valid
>   *   (may be tested with json_path_validate).
> + *
> + * When is_weak_cmp flag is set, strings are assumed to be equal
> + * when all b path tokens are present in a path.
>   */

Please add an example. 
>  int

Are you sure you want a flag an existing function and not an
entirely separate one?

>  json_path_cmp(const char *a, int a_len, const char *b, int b_len,
> -	      int index_base);
> +	      bool is_weak_cmp, int index_base);
>  
>  /**
>   * Check if the passed JSON path is valid.
> diff --git a/test/unit/json.c b/test/unit/json.c
> index 1db7e9364..10704451b 100644
> --- a/test/unit/json.c
> +++ b/test/unit/json.c
> @@ -479,12 +479,12 @@ test_path_cmp()
>  		{"Data[1][\"Info\"].fname[1]", -1},
>  	};
>  	header();
> -	plan(lengthof(rc) + 6);
> +	plan(lengthof(rc) + 8);
>  	for (size_t i = 0; i < lengthof(rc); ++i) {
>  		const char *path = rc[i].path;
>  		int errpos = rc[i].errpos;
>  		int rc = json_path_cmp(a, a_len, path, strlen(path),
> -				       INDEX_BASE);
> +				       false, INDEX_BASE);
>  		if (rc > 0) rc = 1;
>  		if (rc < 0) rc = -1;
>  		is(rc, errpos, "path cmp result \"%s\" with \"%s\": "
> @@ -493,7 +493,7 @@ test_path_cmp()
>  	char *multikey_a = "Data[*][\"FIO\"].fname[*]";
>  	char *multikey_b = "[\"Data\"][*].FIO[\"fname\"][*]";
>  	int ret = json_path_cmp(multikey_a, strlen(multikey_a), multikey_b,
> -				strlen(multikey_b), INDEX_BASE);
> +				strlen(multikey_b), false, INDEX_BASE);
>  	is(ret, 0, "path cmp result \"%s\" with \"%s\": have %d, expected %d",
>  	   multikey_a, multikey_b, ret, 0);
>  
> @@ -510,6 +510,13 @@ test_path_cmp()
>  	   "test json_path_is_multikey: multikey path");
>  	is(ret, 8, "test json_path_is_multikey: multikey path %d/%d", ret, 8);
>  
> +	is(json_path_cmp(a, a_len, multikey_b, ret, false, INDEX_BASE) == 0, false,
> +	   "test json_path_cmp  with is_weak_cmp = false: compare %s and %.*s",
> +	   a, ret, multikey_b);
> +	is(json_path_cmp(a, a_len, multikey_b, ret, true, INDEX_BASE) == 0, true,
> +	   "test json_path_cmp with is_weak_cmp = true: compare %s and %.*s",
> +	   a, ret, multikey_b);

The tests are vastly insufficient. Left empty path, two
empty paths, right empty paths, the subpath is in the middle,
front or tail, a path with wildcards, two paths with wildcards,
 multiple wildcards.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov