json_extract and json_extract_scalar allow invalid JsonPaths (without $ at the start)

[mbasmanova]

Json Path seems to require all paths to start with '$': https://goessner.net/articles/JsonPath/

However, json_extract and json_extract_scalar functions treat leading $ as optional.

Looks like this "bug" was introduced in dbe2ed9#diff-c1770855d6e7c3f3e05245346d9f1ee514df7310d141019e51631124706109e6

Before this change there was an explicit check for leading $:

    @VisibleForTesting
    public static ImmutableList<Object> tokenizePath(String path)
    {
        checkCondition(!isNullOrEmpty(path), INVALID_FUNCTION_ARGUMENT, "Invalid JSON path: '%s'", path);
        checkCondition(path.charAt(0) == '$', INVALID_FUNCTION_ARGUMENT, "JSON path must start with '$': '%s'", path);
...

After, this change, leading $ became optional:

     public JsonPathTokenizer(String path)
    {
        this.path = checkNotNull(path, "path is null");

        // skip the start token
        match('$');
    } 

Velox currently requires JSON Paths to start with '$'.

CC: @tdcmeehan @Yuhta @kagamiori @kgpai @amitkdutta

[mbasmanova]

Looks like the reason for Presto to allow such paths is fallback logic to Jayway:

presto-main/src/main/java/com/facebook/presto/operator/scalar/JsonPath.java

    public static JsonPath build(String pattern)
    {
        try {
            return buildPresto(pattern);
        }
        catch (PrestoException ex) {
            if (ex.getErrorCode() == INVALID_FUNCTION_ARGUMENT.toErrorCode()) {
                return buildJayway(pattern);
            }
            throw ex;
        }
    }

com.jayway.jsonpath.JsonPath.compile(path) happily translates 'foo' path into '$[foo]' path.

[mbasmanova]

Jayway has logic to check if path starts with '$' or '@' and if not to prepend the path with '$.' before compiling.

com/jayway/jsonpath/json-path/2.6.0/json-path-2.6.0.jar!/com/jayway/jsonpath/internal/path/PathCompiler.class

    public static Path compile(String path, Predicate... filters) {
        try {
            CharacterIndex ci = new CharacterIndex(path);
            ci.trim();
            if (ci.charAt(0) != '$' && ci.charAt(0) != '@') {
                ci = new CharacterIndex("$." + path);
                ci.trim();
            }

            if (ci.lastCharIs('.')) {
                fail("Path must not end with a '.' or '..'");
            }

            LinkedList<Predicate> filterStack = new LinkedList(Arrays.asList(filters));
            Path p = (new PathCompiler(ci, filterStack)).compile();
            return p;

[mbasmanova]

Also, JSON Path seems to allow either dot-notation ($.store.book[0].title) or bracket-notation ($['store']['book'][0]['title']), but Jayway allows a mix: $.['store'].['book'].[0].['title']

presto:di> SELECT json_extract_scalar('[{"foo": 1}, {"bar": 2}]', '$.0.foo');
 _col0
-------
 1

presto:di> SELECT json_extract_scalar('[{"foo": 1}, {"bar": 2}]', '$[0]foo');
 _col0
-------
 1

presto:di> SELECT json_extract_scalar('[{"foo": 1}, {"bar": 2}]', '$[0].foo');
 _col0
-------
 1

presto:di> SELECT json_extract_scalar('[{"foo": 1}, {"bar": 2}]', '$[0].[''foo'']');
 _col0
-------
 1

presto:di> SELECT json_extract_scalar('[{"foo": 1}, {"bar": 2}]', '$[0][''foo'']');
 _col0
-------
 1

[mbasmanova]

Jayway has logic to check if path starts with '$' or '@' and if not to prepend the path with '$.' before compiling.

This logic seems problematic as it produces some unexpected results. ".[0]" path is converted to "..[0]" which performs deep scan and returns a list of first elements for ALL arrays in the JSON document.

presto:di> SELECT json_extract('[1, 2, [10, 20, [100]]]', '$[0]');
 _col0
-------
 1

presto:di> SELECT json_extract('[1, 2, [10, 20, [100]]]', '$.[0]');
 _col0
-------
 1

presto:di> SELECT json_extract('[1, 2, [10, 20, [100]]]', '.[0]');
   _col0
------------
 [1,10,100]

I think it is fine to change Velox to treat leading '$' as optional, but I don't think Velox should prepend '$.' to the path when leading '$' is missing.

CC: @sviscaino

[mbasmanova]

I made the following changes in Velox:

• Make leading '$.' optional in JSON paths accepted by json_extract[_scalar] Presto functions facebookincubator/velox#9584
• Allow redundant dots in JSON paths accepted by json_extract[_scalar] Presto facebookincubator/velox#9585