MIME-Version: 1.0
References: <1cee81f3-312f-30e3-0282-c4df7ee72f93@xs4all.nl>
 <202403041503.5momp6knupgo@alvherre.pgsql> <CA+HiwqGtdVCg-KboUUU=svwukwXGT4EOFOk3eCZLz6h-d6YLNw@mail.gmail.com>
In-Reply-To: <CA+HiwqGtdVCg-KboUUU=svwukwXGT4EOFOk3eCZLz6h-d6YLNw@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Tue, 5 Mar 2024 12:42:17 +0800
Message-ID: <CACJufxERsm85E4NSC0iQNG34=wfV4=htchtx6D=hn+k4mtFAuQ@mail.gmail.com>
Subject: Re: remaining sql/json patches
To: Amit Langote <amitlangote09@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Erik Rijkers <er@xs4all.nl>, 
	Andres Freund <andres@anarazel.de>, Andrew Dunstan <andrew@dunslane.net>, 
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CACJufxERsm85E4NSC0iQNG34%3DwfV4%3Dhtchtx6D%3Dhn%2Bk4mtFAuQ%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 5, 2024 at 9:22=E2=80=AFAM Amit Langote <amitlangote09@gmail.co=
m> wrote:
>
> Thanks for the heads up.  Attaching rebased patches.
>

Walking through the v41-0001-Add-SQL-JSON-query-functions.patch documentati=
on.
I found some minor cosmetic issues.

+       <para>
+        <literal>select json_query(jsonb '{"a": "[1, 2]"}', 'lax $.a'
RETURNING int[] OMIT QUOTES);</literal>
+        <returnvalue></returnvalue>
+       </para>
this example is not so good, it returns NULL, makes it harder to
render the result.

+   <replaceable>context_item</replaceable> (the document); seen
+   <xref linkend=3D"functions-sqljson-path"/> for more details on what
+   <replaceable>path_expression</replaceable> can contain.
"seen" should be "see"?

+       <para>
+        This function must return a JSON string, so if the path expression
+        returns multiple SQL/JSON items, you must wrap the result using th=
e
+        <literal>WITH WRAPPER</literal> clause.  If the wrapper is
"must" may be not correct?
since we have a RETURNING clause.
"generally" may be more accurate, I think.
maybe we can rephrase the sentence:
+        This function generally return a JSON string, so if the path expre=
ssion
+        yield multiple SQL/JSON items, you must wrap the result using the
+        <literal>WITH WRAPPER</literal> clause

+        is spcified, the returned value will be of type <type>text</type>.
+        If no <literal>RETURNING</literal> is spcified, the returned value=
 will
two typos, and should be "specified".

+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal> and <literal>ON ERROR</literal> behav=
ior
+        is <literal>ON ERROR</literal>, an error is generated if it yields=
 no
+        items.

may be the following:
+        Note that if the <replaceable>path_expression</replaceable>
+        is <literal>strict</literal> and <literal>ON ERROR</literal> behav=
ior
+        is <literal>ERROR</literal>, an error is generated if it yields no
+        items.

most of the place, you use
 <replaceable>path_expression</replaceable>
but there are two place you use:
<type>path_expression</type>
I guess that's ok, but the appearance is different.
 <replaceable> more prominent. Anyway, it is a minor issue.

+        <function>json_query</function>.  Note that scalar strings returne=
d
+        by <function>json_value</function> always have their quotes remove=
d,
+        equivalent to what one would get with <literal>OMIT QUOTES</litera=
l>
+        when using <function>json_query</function>.

I think we can simplify it like the following:

+        <function>json_query</function>.  Note that scalar strings returne=
d
+        by <function>json_value</function> always have their quotes remove=
d,
+        equivalent to <literal>OMIT QUOTES</literal>
+        when using <function>json_query</function>.