Re: jsonpath

Attached 36th version of the patches.

On 03.03.2019 21:08, Tomas Vondra wrote:

Hi,

Here are some initial comments from a review of the 0001 part. I plan to
do more testing on a large data set and additional round of review over
the next week. FWIW I've passed this through valgrind and the usual
battery of regression tests, and there were no issues.

Thanks again for your review.

I haven't looked at 0002 yet, but it seems fairly small (especially
compared to 0001).

func.sgml
=========

1) I see the changes removed <indexterm zone="functions-json"> for some
reason. Is that intentional?

Fixed.

2) <command>WHERE</command>
   We generally tag <literal>WHERE</literal>, not <command>.

Fixed.

3) Filter expressions are applied from left to right
   Perhaps s/applied/evaluated/ in reference to expressions?

Fixed.

4) The result of the filter expression may be true, false, or unknown.
   It's not entirely clear to me what "unknown" means here. NULL, or   something else? There's a section in the SQL/JSON standard   explaining this (page 83), but perhaps we should explain it a bit   here too?
   The standard says "In the SQL/JSON data model, there are no SQL   nulls, so Unknown is not part of the SQL/JSON data model." so I'm a   bit confused what "unknown" references to. Maybe some example?
   Also, what happens when the result is unknown?

"unknown" refers here to ordinary three-valued logical Unknown, which is
represented in SQL by NULL.

JSON path expressions return sequences of SQL/JSON items, which are defined by
SQL/JSON data model.  But JSON path predicates (logical expressions), which are
used in filters, return three-valued logical values: False, True, or Unknown.

Filters accept only items for which the filter predicate returned True. False
and Unknown results are skipped.

Unknown can be checked with IS UNKNOWN predicate:

SELECT jsonb_path_query_array('[1, "1", true, null]',                             '$[*] ? ((@ < 2) is unknown)');jsonb_path_query_array 
------------------------["1", true]
(1 row)

Comparison of JSON nulls to non-nulls returns always False, not Unknown (see
SQL/JSON data model). Comparison of non-comparable items return Unknown.

5) There's an example showing how to apply filter at a certain level,
using the @ variable, but what if we want to apply multiple filters at
different levels? Would it make sense to add such example?

Examples were added.

Filters can be nested, but by the standard it is impossible to reference item
of the outer filter, because @ always references current item of the innermost
filter.  We have additional patch with a simple jsonpath syntax extension for 
this case -- @N:@0, like @, references innermost item@1 references item one level upper@2 references item two levels upper

For example, selecting all objects from array
'[{"vals": [1,2,3], "val": 2}, {"vals": [4,5], "val": 6}]'

having in their .vals[] element greater than their .val field:
'$[*] ? (@.vals[*] ? (@ > @1.val))'

It is impossible to do this by the standard in the single jsonpath expression.


Also there is idea to use lambda expressions with ECMAScript 6 syntax in
filters and user method (see below):

'$[*] ? (obj => obj.vals[*] ? (val => val > obj.val))'

I already have a patch implementing lambda expressions, which were necessary
for implementaion of built-in/user-written functions and methods like
.map(), reduce(), max().  I can post it again if it is interesting.

6) ... extensions of the SQL/JSON standard
   I'm not sure what "extension" is here. Is that an extension defined   in the SQL standard, or an additional PostgreSQL functionality not   described in the standard? (I assume the latter, just checking.)

Yes, this is additional functionality.

 "Writing the path as an expression is also valid"   I have moved this into SQL/JSON patch, because it is related only path   specification in SQL/JSON functions.

7) There are references to "SQL/JSON sequences" without any explanation
what it means. Maybe I'm missing something obvious, though.

SQL/JSON sequence is a sequence of SQL/JSON items.
The corresponding definition was added.

8) Implicit unwrapping in the lax mode is not performed in the following
cases:
   I suggest to reword it like this:

In the lax mode, implicit unwrapping is not performed when:

Fixed. I also decided to merge this paragraph with paragraph describing 
auto-unwrapping.

9) We're not adding the datetime() method for now, due to the issues
with timezones. I wonder if we should add a note why it's missing to the
docs ...

The corresponding note was added.

10) I'm a bit puzzled though, because the standard says this in the
description of type() function on page 77
   If I is a datetime, then “date”, “time without time zone”, “time   with time zone”, “timestamp without time zone”, or “timestamp with   time zone”, as appropriate.

But I see our type() function does not return anything like that (which
I presume is independent of timezone stuff). But I see jsonb.h has no
concept of datetime values, and the standard actually says this in the
datetime() section
   JSON has no datetime types. Datetime values are most likely stored   in character strings.

Considering this, the type() section makes little sense, no?

According to the SQL/JSON data model, SQL/JSON items can be null, string, 
boolean, numeric, and datetime.

Datetime items exists only at execution time, they are serialized into JSON
strings when the resulting SQL/JSON item is converted into jsonb. After removal
of .datetime() method, support of datetime SQL/JSON items was removed from
jsonpath executor too.

Numeric items can be of any numeric type.  In PostgreSQL we have the following
numeric datatypes: integers, floats and numeric.  But our jsonpath executor
supports now only numeric-typed items, because this is only type we can get
directly from jsonb.  Support for other numeric datatypes (float8 is most
necessary, because it is produced by .double() item method) can be added by
extending JsonbValue or by introducing struct JsonItem in executor, having
JsonbValue as a part (see patch #4 in v34).

jsonb_util.c
============

I see we're now handling NaN values in convertJsonbScalar(). Isn't it
actually a bug that we don't do this already? Or is it not needed for
some reason?

Numeric JsonbValues created outside of jsonpath executor cannot be NaN, because
this case in explicitly handled in JsonbValue-producing functions.  For example,
datum_to_jsonb() which is used in to_jsonb(), jsonb_build_array() and others
converts Inf and NaN into JSON strings.  But jsonb_plperl and jsonb_plpython
transforms do not allow NaNs (I think it is needed only for consistency of
"PL => jsonb => PL" roundtrip).

In our jsonpath executor we can produce NaNs and we should not touch them before
conversion to the resulting jsonb.  Moreover, in SQL/JSON functions numeric
SQL/JSON item can be directly converted into numeric RETURNING type.  So, I
decided to add a check for NaN to the low-level convertJsonbScalar() instead of
checking items before every call to JsonbValueToJsonb() or pushJsonbValue().
But after introduction of struct JsonItem (see patch #4) introduction there will
appropriate place for this check -- JsonItemToJsonbValue().

==========

I suppose this should say "jsonpath version number" instead?
   elog(ERROR, "unsupported jsonb version number %d", version);

Fixed.

A bunch of additional comments, after looking at the patch a bit today.
All are mostly minor, and sometime perhaps a matter of preference.


1) There's a mismatch between the comment and actual function name for
jsonb_path_match_opr and jsonb_path_exists_opr(). The comments say
"_novars" instead.

Fixed.

2) In a couple of switches the "default" case does a return with a
value, following elog(ERROR). So it's practically unreachable, AFAICS
it's fine without it, and we don't do this elsewhere. And I don't get
any compiler warnings if I remove it either.

Examples:

JsonbTypeName
   default:       elog(ERROR, "unrecognized jsonb value type: %d", jbv->type);       return "unknown";

jspOperationName
   default:       elog(ERROR, "unrecognized jsonpath item type: %d", type);       return NULL;

compareItems
   default:       elog(ERROR, "unrecognized jsonpath operation: %d", op);       return jpbUnknown;

It seems to be a standard practice in jsonb code, so we followed it.

3) jsonpath_send is using makeStringInfo() for a value that is not
returned - IMHO it should use regular stack-allocated variable and use
initStringInfo() instead

Fixed.

4) the version number should be defined/used as a constant, not as a
magic constant somewhere in the code

Fixed.

5) Why does jsonPathToCstring do this?
   appendBinaryStringInfo(out, "strict ", 7);

Why not to use regular appendStringInfoString()? What am I missing?

appendStringInfoString() is a bit slower than appendBinaryStringInfo() 
because to strlen() call inside it.

6) comment typo: "Aling StringInfo"

Fixed.

7) alignStringInfoInt() should explain why we need this and why INTALIGN
is the right alignment.

Comment was added.

8) I'm a bit puzzled by what flattenJsonPathParseItem does with 'next'
   I don't quite understand what it's doing with 'next' value?
   /*    * Actual value will be recorded later, after next and children    * processing.    */   appendBinaryStringInfo(buf,                          (char *) &next, /* fake value */                          sizeof(next));

Perhaps a comment explaining it (why we need a fake value at all?) would
be a good idea here.

No fake value is needed here, zero placeholder is enough. I have refactored
this using new function reserveSpaceForItemPointer().

9) I see printJsonPathItem is only calling check_stack_depth while
flattenJsonPathParseItem also calls CHECK_INTERRUPTS. Why the
difference, considering they seem about equally expensive?

CHECK_INTERRUPT() was added to printJsonPathItem() too.

10) executeNumericItemMethod is missing a comment (unlike the other
executeXXX functions)

Comment was added.

11) Wording of some of the error messages in the execute methods seems a
bit odd. For example executeNumericItemMethod may complain that it
... is applied to not a numeric value

but perhaps a more natural wording would be
... is applied to a non-numeric value

And similarly for the other execute methods. But I'm not a native
speaker, so perhaps the original wording is just fine.

Error messages were changed on the advice of Robert Haas to:
"... can only be applied to a numeric value"

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company