Re: Patch: Improve Boolean Predicate JSON Path Docs

On Oct 14, 2023, at 19:51, Erik Wienhold <ewie@ewie.name> wrote:

> Thanks for putting this together.  See my review at the end.

Appreciate the speedy review!

> Nice.  This really does help to make some sense of it.  I checked all
> queries and they do work out except for two queries where the path
> expression string is not properly quoted (but the intended output is
> still correct).

🤦🏻‍♂️

>> Follow-ups I’d like to make:
>>
>> 1. Expand the modes section to show how the types of results can vary
>> depending on the mode, thanks to the flattening. Examples:
>>
>> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}', '$.a ?(@[*] > 2)');
>> jsonb_path_query
>> ------------------
>> 3
>> 4
>> 5
>> (3 rows)
>>
>> david=# select jsonb_path_query('{"a":[1,2,3,4,5]}',  'strict $.a ?(@[*] > 2)');
>> jsonb_path_query
>> ------------------
>> [1, 2, 3, 4, 5]
>>
>> 2. Improve the descriptions and examples for @?/jsonb_path_exists()
>> and @@/jsonb_path_match().
>
> +1

I planned to submit these changes in a separate patch, based on Tom Lane’s suggestion[1]. Would it be preferred to add
themto this patch? 

> Perhaps make it explicit that the reader must run this in psql in order
> to use \set and :'json' in the ensuing samples?  Some of the existing
> examples already use psql output but they do not rely on any psql
> features.

Good call, done.

> This should use <screen>, <userinput>, and <computeroutput> if it shows
> a psql session, e.g.:
>
> <screen>
> <userinput>select jsonb_path_query(:'json', '$.track.segments');</userinput>
> <computeroutput>
>                                                                         jsonb_path_query
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
> [{"HR": 73, "location": [47.763, 13.4034], "start time": "2018-10-14 10:05:14"}, {"HR": 135, "location": [47.706,
13.2635],"start time": "2018-10-14 10:39:21"}] 
> </computeroutput>
> </screen>

I pokwds around, and it appears the computeroutput bit is used for function output. So I followed the precedent in
queries.sgml[2]and omitted the computeroutput tags but added prompt, e.g., 

<screen>
<prompt>=></prompt> <userinput>select jsonb_path_query(:'json', 'strict $.**.HR');</userinput>
jsonb_path_query
------------------
73
135
</screen>

> Also the cast to jsonb is not necessary and only adds clutter IMO.

Right, removed them all in function calls.

>> +     <para>
>> +      Predicate-only path expressions are necessary for implementation of the
>> +      <literal>@@</literal> operator (and the
>> +      <function>jsonb_path_match</function> function), and should not be used
>> +      with the <literal>@?</literal> operator (or
>> +      <function>jsonb_path_exists</function> function).
>> +     </para>
>> +
>> +     <para>
>> +      Conversely, non-predicate <type>jsonpath</type> expressions should not be
>> +      used with the <literal>@@</literal> operator (or the
>> +      <function>jsonb_path_match</function> function).
>> +     </para>
>> +    </sect4>
>
> Both paras should be wrapped in a single <note> so that they stand out
> from the rest of the text.  Maybe even <warning>, but <note> is already
> used on this page for things that I'd consider warnings.

Agreed. Would be good if we could teach these functions and operators to reject path expressions they don’t support.

>> +    <sect4 id="jsonpath-regular-expression-deviation">
>> +    <title>Regular Expression Interpretation</title>
>> +     <para>
>> +      There are minor differences in the interpretation of regular
>> +      expression patterns used in <literal>like_regex</literal> filters, as
>> +      described in <xref linkend="jsonpath-regular-expressions"/>.
>> +     </para>
>> +    </sect4>
>
> <sect3 id="devations-from-the-standard"> should be closed here,
> otherwise the docs won't build.  This can be checked with
> `make -C doc/src/sgml check`.

Thanks. That produces a bunch of warnings for postgres.sgml and legal.sgml (and a failure to load the docbook DTD), but
func.sgmlis clean now. 

> `git diff --check` shows a couple of lines with trailing whitespace
> (mostly psql output).

I must’ve cleaned those after I sent the patch, good now. Updated patch attached, this time created by `git
format-patch-v2`. 

Best,

David

[1] https://www.postgresql.org/message-id/1229727.1680535592%40sss.pgh.pa.us
[2] https://www.postgresql.org/docs/current/queries-table-expressions.html#QUERIES-JOIN