Re: jsonpath - Mailing list pgsql-hackers

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.

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?

2) <command>WHERE</command>

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

3) Filter expressions are applied from left to right

    Perhaps s/applied/evaluated/ in reference to expressions?

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?


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?

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.)


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

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:

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 ...

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?


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?

jsonpath.c
==========

I suppose this should say "jsonpath version number" instead?

    elog(ERROR, "unsupported jsonb version number %d", version);


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services