Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions - Mailing list pgsql-hackers

Thanks for the readthrough.

On Sat, Jul 6, 2024 at 11:56 AM jian he <jian.universality@gmail.com> wrote:
> json_exists
> "Returns true if the SQL/JSON path_expression applied to the
> context_item using the PASSING values yields any items."
> now you changed to
> <<
> Returns true if the SQL/JSON path_expression applied to the
> context_item. path_expression can reference variables named in the
> PASSING clause.
> <<
>  Is it wrong?

Yes, I mistakenly dropped "doesn't yield any items."

>         For both <literal>ON EMPTY</literal>and <literal>ON ERROR</literal>,
>         specifying <literal>ERROR</literal> will cause an error to be
> thrown with
>         the appropriate message. Other options include returning a SQL NULL, an
> need one more whitespace. should be:
>         For both <literal>ON EMPTY</literal> and <literal>ON ERROR</literal>,

Fixed.

>         Note that an error will be thrown if multiple values are returned and
>         <literal>WITHOUT WRAPPER</literal> is specified.
> since not specify error on error, then no error will be thrown. maybe
> rephrase to
>         It will be evaulated as error if multiple values are returned and
>         <literal>WITHOUT WRAPPER</literal> is specified.

Done.

>        <para>
>         For both <literal>ON EMPTY</literal> and <literal>ON ERROR</literal>,
>         specifying <literal>ERROR</literal> will cause an error to be
> thrown with
>         the appropriate message. Other options include returning a SQL NULL, an
>         empty array or object (array by default), or a user-specified expression
>         that can be coerced to jsonb or the type specified in
> <literal>RETURNING</literal>.
>         The default when <literal>ON EMPTY</literal> or <literal>ON
> ERROR</literal>
>         is not specified is to return a SQL NULL value when the respective
>         situation occurs.
>        </para>
> in here, "empty array or object (array by default)",
> I don't think people can understand the meaning of "(array by default)" .
>
> "or a user-specified expression"
> maybe we can change to
> "or a user-specified
>         <literal>DEFAULT</literal> <replaceable>expression</replaceable>"
> I think "user-specified expression" didn't have much link with
> "<literal>DEFAULT</literal> <replaceable>expression</replaceable>"

Yes, specifying each option's syntax in parenthesis makes sense.

> <replaceable>path_expression</replaceable> can reference variables named
>         in the <literal>PASSING</literal> clause.
> do we need "The <replaceable>path_expression</replaceable>"?

Fixed.

> also maybe we can add
> + In <literal>PASSING</literal> clause, <replaceable>varname</replaceable> is
> +        the variables name, <replaceable>value</replaceable> is the
> + variables' value.

Instead of expanding the description of the PASSING clause in each
function's description, I've moved its description to the top
paragraph with slightly different text.

> we can add a PASSING clause example from sqljson_queryfuncs.sql ,
> since all three functions can use it.

Done.

> JSON_VALUE:
> <<an error is thrown if that's not the case (though see the discussion
> of ON ERROR below).
> then
> << The ON ERROR and ON EMPTY clauses have similar semantics as
> mentioned in the description of JSON_QUERY, except the set of values
> returned in lieu of throwing an error is different.
>
> you first refer "below", then director to JSON_QUERY on error, on
> empty description.
> is the correct usage of "below"?
> "(though see the discussion of ON ERROR below)."
> i am not sure the meaning of "though" even watched this
> https://www.youtube.com/watch?v=r-LphuCKQ0Q

I've replaced the sentence with "(though see the discussion of ON
ERROR below)" with this:

"getting multiple values will be treated as an error."

No need to reference the ON ERROR clause with that wording.

> +   <replaceable>context_item</replaceable> can be any character string that
> +   can be succesfully cast to <type>jsonb</type>.
>
> typo: "succesfully", should be "successfully"

Fixed.

> maybe rephrase it to:
> +   <replaceable>context_item</replaceable> can be jsonb type or any
> character string that
> +   can be successfully cast to <type>jsonb</type>.

Done.

> +        <literal>ON EMPTY</literal> expression (that is caused by empty result
> +        of <replaceable>path_expression</replaceable>evaluation).
> need extra white space, should be
> +        of <replaceable>path_expression</replaceable> evaluation).

Fixed.

> +        The default when <literal>ON EMPTY</literal> or <literal>ON
> ERROR</literal>
> +        is not specified is to return a SQL NULL value when the respective
> +        situation occurs.
> Correct me if I'm wrong.
> we can just say:
> +        The default when <literal>ON EMPTY</literal> or <literal>ON
> ERROR</literal>
> +        is not specified is to return an SQL NULL value.

Agreed.

> another tiny issue.
>
> -        <literal>select json_query(jsonb '{"a": "[1, 2]"}', 'lax $.a'
> OMIT QUOTES);</literal>
> +        <literal>JSON_QUERY(jsonb '{"a": "[1, 2]"}', 'lax $.a' OMIT
> QUOTES);</literal>
>          <returnvalue>[1, 2]</returnvalue>
>         </para>
>         <para>
> -        <literal>select json_query(jsonb '{"a": "[1, 2]"}', 'lax $.a'
> RETURNING int[] OMIT QUOTES ERROR ON ERROR);</literal>
> +        <literal>JSON_QUERY(jsonb '{"a": "[1, 2]"}', 'lax $.a'
> RETURNING int[] OMIT QUOTES ERROR ON ERROR);</literal>
>
> These two example queries don't need semicolons at the end?

Fixed.

Also, I've removed the following sentence in the description of
JSON_EXISTS, because a) it seems out of place

-       <para>
-        Note that if the <replaceable>path_expression</replaceable> is
-        <literal>strict</literal> and <literal>ON ERROR</literal> behavior is
-        <literal>ERROR</literal>, an error is generated if it yields no items.
-       </para>

and b) does not seem correct:

SELECT JSON_EXISTS(jsonb '{"key1": [1,2,3]}', 'strict $.key1[*] ? (@ >
3)' ERROR ON ERROR);
 json_exists
-------------
 f
(1 row)

path_expression being strict or lax only matters inside
jsonpath_exec.c, not in ExecEvalJsonPathExpr().

Updated patch attached.

--
Thanks, Amit Langote