Thread: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
Hey!
Lots of SQL/JSON threads going about. This one is less about technical correctness and more about usability of the documentation. Though in writing this I am finding some things that aren't quite clear. I'm going to come back with those on a follow-on post once I get a chance to make my second pass on this. But for the moment just opening it up to a content and structure review.
Please focus on the text changes. It passes "check-docs" but I still need to work on layout and stuff in html (markup, some more links).
Thanks!
David J.
p.s. v1 exists here (is just the idea of using basically variable names in the function signature and minimizing direct syntax in the table);
Attachment
Hi David, On Tue, Jun 25, 2024 at 3:47 PM David G. Johnston <david.g.johnston@gmail.com> wrote: > > Hey! > > Lots of SQL/JSON threads going about. This one is less about technical correctness and more about usability of the documentation. Though in writing this I am finding some things that aren't quite clear. I'm going to come back with thoseon a follow-on post once I get a chance to make my second pass on this. But for the moment just opening it up to acontent and structure review. > > Please focus on the text changes. It passes "check-docs" but I still need to work on layout and stuff in html (markup,some more links). > > Thanks! > > David J. > > p.s. v1 exists here (is just the idea of using basically variable names in the function signature and minimizing directsyntax in the table); > > https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com Thanks for writing the patch. I'll take a look at this next Monday. -- Thanks, Amit Langote
On Fri, Jun 28, 2024 at 2:56 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Jun 25, 2024 at 3:47 PM David G. Johnston > <david.g.johnston@gmail.com> wrote: > > > > Hey! > > > > Lots of SQL/JSON threads going about. This one is less about technical correctness and more about usability of the documentation. Though in writing this I am finding some things that aren't quite clear. I'm going to come back with thoseon a follow-on post once I get a chance to make my second pass on this. But for the moment just opening it up to acontent and structure review. > > > > Please focus on the text changes. It passes "check-docs" but I still need to work on layout and stuff in html (markup,some more links). > > > > Thanks! > > > > David J. > > > > p.s. v1 exists here (is just the idea of using basically variable names in the function signature and minimizing directsyntax in the table); > > > > https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com > > Thanks for writing the patch. I'll take a look at this next Monday. I've attached a delta (0002) against your patch, wherein I've kept most of the structuring changes you've proposed, but made changes such as: * use tags consistently * use language matching the rest of func.sgml, IMO * avoid repetition (eg. context_item described both above and below the table) * correcting some factual discrepancies (eg. json_value never returns json null) * avoid forward references * capitalize function names, SQL keywords in examples as requested in a previous review [1] Maybe we could still polish this some more. -- Thanks, Amit Langote [1] https://www.postgresql.org/message-id/CAA-aLv7Dfy9BMrhUZ1skcg%3DOdqysWKzObS7XiDXdotJNF0E44Q%40mail.gmail.com
Attachment
hi.
the following review is based on v2-0001, v2-0002.
"context_item can be a JSON document passed as a value of type json,
jsonb document, a character or an UTF8- endoded bytea string."
is wrong?
e.g. SELECT JSON_EXISTS( NULL::bytea, 'lax $.a[5]' ERROR ON ERROR)
check following query:
select oid, typtype , typname from pg_type where typcategory = 'S';
I think a more accurate description would be:
"context_item must be a JSON document passed as a value of type json,
jsonb document, a character string type(text, name, bpchar, varchar)"
do we need to mention domain over these types?
-------------------------------------
JSON_EXISTS
Returns true if the SQL/JSON path_expression possibly referencing the
variables in variable_definitions applied to the context_item yields
any items.
I am not native English speaker, so I found it hard to comprehend.
I can understand it like:
"Returns true if the SQL/JSON path_expression (possibly referencing
the variables in variable_definitions) applied to the context_item
yields any items."
maybe we can write it into two sentences, or
"Returns true if the SQL/JSON path_expression applied to the
context_item yields any items."
because you already mentioned "path_expression can also contain
variables whose values are specified using the variable_definitions
clause described below." in the top level.
-------------------------------------
The JSON_QUERY and JSON_VALUE functions are polymorphic in their
output type with the returning_clause clause dictating what that type
is.
how about
The JSON_QUERY and JSON_VALUE functions output type can be vary, using
returning_clause specify the desired data type.
-------------------------------------
your doc: JSON_VALUE "If path_expression points to a JSON null,
JSON_VALUE returns a SQL NULL."
`SELECT JSON_VALUE(jsonb 'null', '$');` here, the path_expression
points to '$' which is not json null?
so i like to change it to
"If the extracted value is a JSON null, an SQL NULL value will return."
-------------------------------------
inconsistency:
JSON_QUERY: <returnvalue></returnvalue> { <type>jsonb</type> |
<replaceable>return_data_type</replaceable> }
JSON_VALUE: <returnvalue></returnvalue> { <type>text</type> |
<varname>return_data_type</varname> }
-------------------------------------
{{For JSON_EXISTS (... on_error_boolean), alternative can be: ERROR,
UNKNOWN, TRUE, FALSE.
For JSON_QUERY (... on_error_set on_empty_set), alternative can be:
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT followed by an
expression.
For JSON_VALUE (... on_error_set on_empty_set), alternative can be:
ERROR, NULL, or DEFAULT followed by an expression.
}}
i am not sure what does there dot means here, in the synopsis section,
three dots is significant.
Also if I understand it correctly, JSON_EXISTS can only have on_error,
then I am more confused with ``JSON_EXISTS (... on_error_boolean)``
Overall, I found this approach makes the synopsis scattered, it's not
easy to see the full picture.
for example:
```
JSON_VALUE ( context_item, path_expression [variable_definitions]
[return_type] [on_empty_value] [on_error_value]) → { text |
return_data_type }
```
this way it is not easy to find out that RETURNING is a keyword.
Currently in master, we can quickly see RETURNING is the keyword, the
master is kind of condense, though.
but if you are insistent with your approach, then that is fine for me.
still based on v2-0001, v2-0002.
picture attached.
as you can see from the curly braces,
```
{ KEEP | OMIT } QUOTES [ ON SCALAR STRING ]
```
we must choose one, "KEEP" or "OMIT".
but the wrapping_clause:
```
WITHOUT [ARRAY] WRAPPER
WITH [UNCONDITIONAL] [ARRAY] WRAPPER
WITH CONDITIONAL [ARRAY] WRAPPER
```
this way, didn't say we must choose between one in these three.
-----------
on_error_boolean
on_error_set
on_error_value
on_empty_set
on_empty_value
alternative ON { ERROR | EMPTY }
````
didn't explain on_error_value, on_empty_value.
why not just on_error_clause, on_empty_clause?
-------
<<<quoted paragraph
When JSON_QUERY function produces multiple JSON values, they are
returned as a JSON array. By default, the result values are
unconditionally wrapped even if the array contains only one element.
You can specify the WITH CONDITIONAL variant to say that the wrapper
be added only when there are multiple values in the resulting array.
Or specify the WITHOUT variant to say that the wrapper be removed when
there is only one element, but it is ignored if there are multiple
values.
<<<quoted paragraph
The above paragraph didn't explicitly mention that UNCONDITIONAL is the default.
BTW, by comparing patch with master, I found out:
"""
If the wrapper is UNCONDITIONAL, an array wrapper will always be
applied, even if the returned value is already a single JSON object or
an array. If it is CONDITIONAL, it will not be applied to a single
JSON object or an array. UNCONDITIONAL is the default.
"""
this description seems not right.
if "UNCONDITIONAL is the default", then
select json_query(jsonb '{"a": [1]}', 'lax $.a' with unconditional
array wrapper);
should be same as
select json_query(jsonb '{"a": [1]}', 'lax $.a' );
another two examples with SQL/JSON scalar item:
select json_query(jsonb '{"a": 1}', 'lax $.a' );
select json_query(jsonb '{"a": 1}', 'lax $.a' with unconditional wrapper);
Am I interpreting "UNCONDITIONAL is the default" the wrong way?
Attachment
Hi Jian,
Thanks for the reviews.
On Wed, Jul 3, 2024 at 11:15 AM jian he <jian.universality@gmail.com> wrote:
> Overall, I found this approach makes the synopsis scattered, it's not
> easy to see the full picture.
> for example:
> ```
> JSON_VALUE ( context_item, path_expression [variable_definitions]
> [return_type] [on_empty_value] [on_error_value]) → { text |
> return_data_type }
> ```
> this way it is not easy to find out that RETURNING is a keyword.
> Currently in master, we can quickly see RETURNING is the keyword, the
> master is kind of condense, though.
> but if you are insistent with your approach, then that is fine for me.
Actually, on second thought, I too am finding the new structure
whereby descriptions of various clauses are moved below the table a
bit hard to follow. Descriptions in the table have to use forward
references to bits outside the table and vice versa. Like you, I also
don't like the style used in the new structure for describing various
ON ERROR values that differ based on the function. It seems better to
just list the syntax in the table and then each function's syntax
synopsis tells what values are appropriate to return for that given
function ON ERROR.
So I've decided to resurrect the *other* documentation rewrite patch
that you reviewed back in May.
> "context_item can be a JSON document passed as a value of type json,
> jsonb document, a character or an UTF8- endoded bytea string."
> is wrong?
> e.g. SELECT JSON_EXISTS( NULL::bytea, 'lax $.a[5]' ERROR ON ERROR)
Oops, I was thinking of what can be used in the RETURNING clause.
> check following query:
> select oid, typtype , typname from pg_type where typcategory = 'S';
>
> I think a more accurate description would be:
> "context_item must be a JSON document passed as a value of type json,
> jsonb document, a character string type(text, name, bpchar, varchar)"
> do we need to mention domain over these types?
I don't feel the need to mention every possible type, so I went with:
+ <replaceable>context_item</replaceable> can be any character string that
+ can be succesfully cast to <type>jsonb</type>.
> -------------------------------------
> JSON_EXISTS
> Returns true if the SQL/JSON path_expression possibly referencing the
> variables in variable_definitions applied to the context_item yields
> any items.
> I am not native English speaker, so I found it hard to comprehend.
> I can understand it like:
> "Returns true if the SQL/JSON path_expression (possibly referencing
> the variables in variable_definitions) applied to the context_item
> yields any items."
>
> maybe we can write it into two sentences, or
> "Returns true if the SQL/JSON path_expression applied to the
> context_item yields any items."
> because you already mentioned "path_expression can also contain
> variables whose values are specified using the variable_definitions
> clause described below." in the top level.
Please check the attached patch which contains different text.
> -------------------------------------
> The JSON_QUERY and JSON_VALUE functions are polymorphic in their
> output type with the returning_clause clause dictating what that type
> is.
> how about
> The JSON_QUERY and JSON_VALUE functions output type can be vary, using
> returning_clause specify the desired data type.
Ditto.
> -------------------------------------
> your doc: JSON_VALUE "If path_expression points to a JSON null,
> JSON_VALUE returns a SQL NULL."
> `SELECT JSON_VALUE(jsonb 'null', '$');` here, the path_expression
> points to '$' which is not json null?
> so i like to change it to
> "If the extracted value is a JSON null, an SQL NULL value will return."
I've added a <note> at the bottom:
+ <note>
+ <para>
+ <function>JSON_VALUE()</function> returns SQL NULL if
+ <replaceable>path_expression</replaceable> returns a JSON
+ <literal>null</literal>, whereas <function>JSON_QUERY()</function> returns
+ the JSON <literal>null</literal> as is.
+ </para>
+ </note>
> -------------------------------------
> inconsistency:
> JSON_QUERY: <returnvalue></returnvalue> { <type>jsonb</type> |
> <replaceable>return_data_type</replaceable> }
> JSON_VALUE: <returnvalue></returnvalue> { <type>text</type> |
> <varname>return_data_type</varname> }
No longer in the patch.
> -------------------------------------
> {{For JSON_EXISTS (... on_error_boolean), alternative can be: ERROR,
> UNKNOWN, TRUE, FALSE.
> For JSON_QUERY (... on_error_set on_empty_set), alternative can be:
> ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT followed by an
> expression.
> For JSON_VALUE (... on_error_set on_empty_set), alternative can be:
> ERROR, NULL, or DEFAULT followed by an expression.
> }}
> i am not sure what does there dot means here, in the synopsis section,
> three dots is significant.
> Also if I understand it correctly, JSON_EXISTS can only have on_error,
> then I am more confused with ``JSON_EXISTS (... on_error_boolean)``
Ditto.
> still based on v2-0001, v2-0002.
> picture attached.
>
> as you can see from the curly braces,
> ```
> { KEEP | OMIT } QUOTES [ ON SCALAR STRING ]
> ```
> we must choose one, "KEEP" or "OMIT".
>
> but the wrapping_clause:
> ```
> WITHOUT [ARRAY] WRAPPER
> WITH [UNCONDITIONAL] [ARRAY] WRAPPER
> WITH CONDITIONAL [ARRAY] WRAPPER
> ```
> this way, didn't say we must choose between one in these three.
Ditto.
> -----------
> on_error_boolean
> on_error_set
> on_error_value
> on_empty_set
> on_empty_value
>
> alternative ON { ERROR | EMPTY }
>
> ````
> didn't explain on_error_value, on_empty_value.
> why not just on_error_clause, on_empty_clause?
Ditto.
> -------
> <<<quoted paragraph
> When JSON_QUERY function produces multiple JSON values, they are
> returned as a JSON array. By default, the result values are
> unconditionally wrapped even if the array contains only one element.
> You can specify the WITH CONDITIONAL variant to say that the wrapper
> be added only when there are multiple values in the resulting array.
> Or specify the WITHOUT variant to say that the wrapper be removed when
> there is only one element, but it is ignored if there are multiple
> values.
> <<<quoted paragraph
>
> The above paragraph didn't explicitly mention that UNCONDITIONAL is the default.
> BTW, by comparing patch with master, I found out:
>
> """
> If the wrapper is UNCONDITIONAL, an array wrapper will always be
> applied, even if the returned value is already a single JSON object or
> an array. If it is CONDITIONAL, it will not be applied to a single
> JSON object or an array. UNCONDITIONAL is the default.
> """
> this description seems not right.
> if "UNCONDITIONAL is the default", then
> select json_query(jsonb '{"a": [1]}', 'lax $.a' with unconditional
> array wrapper);
> should be same as
> select json_query(jsonb '{"a": [1]}', 'lax $.a' );
>
> another two examples with SQL/JSON scalar item:
>
> select json_query(jsonb '{"a": 1}', 'lax $.a' );
> select json_query(jsonb '{"a": 1}', 'lax $.a' with unconditional wrapper);
>
> Am I interpreting "UNCONDITIONAL is the default" the wrong way?
Current text is confusing, so I've rewritten the paragraph as:
+ If the path expression may return multiple values, it might
be necessary
+ to wrap those values using the <literal>WITH
WRAPPER</literal> clause to
+ make it a valid JSON string, because the default behavior is
to not wrap
+ them, as if <literal>WITHOUT WRAPPER</literal> were specified. The
+ <literal>WITH WRAPPER</literal> clause is by default taken to mean
+ <literal>WITH UNCONDITIONAL WRAPPER</literal>, which means that even a
+ single result value will be wrapped. To apply the wrapper only when
+ multiple values are present, specify <literal>WITH
CONDITIONAL WRAPPER</literal>.
+ Note that an error will be thrown if multiple values are returned and
+ <literal>WITHOUT WRAPPER</literal> is specified.
So, UNCONDITIONAL is the default as in WITH [UNCONDITIONAL] WRAPPER.
(The default when no wrapping clause is present is WITHOUT WRAPPER as
seen in your example).
Please check the attached. I've also added <itemizedlist> lists as I
remember you had proposed before to make the functions' descriptions a
bit more readable -- I'm persuaded. :-)
--
Thanks, Amit Langote
Attachment
Op 7/5/24 om 14:35 schreef Amit Langote:
> Hi Jian,
>
> Thanks for the reviews.
>
> [v3-0001-SQL-JSON-Various-improvements-to-SQL-JSON-query-f.patch]
i.e., from the patch for doc/src/sgml/func.sgml
Small changes:
4x:
'a SQL' should be
'an SQL'
('a SQL' does never occur in the docs; it's always 'an SQL'; apperently
the 'sequel' pronunciation is out)
'some other type to which can be successfully coerced'
'some other type to which it can be successfully coerced'
'specifies the behavior behavior'
'specifies the behavior'
In the following sentence:
"By default, the result is returned as a value of type <type>jsonb</type>,
though the <literal>RETURNING</literal> clause can be used to return
the original <type>jsonb</type> value as some other type to which it
can be successfully coerced."
it seems to me that this phrase is better removed:
"the original <type>jsonb</type> value as"
thanks,
Erik
On Fri, Jul 5, 2024 at 8:35 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Please check the attached. I've also added <itemizedlist> lists as I
> remember you had proposed before to make the functions' descriptions a
> bit more readable -- I'm persuaded. :-)
>
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?
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>,
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.
<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>"
<replaceable>path_expression</replaceable> can reference variables named
in the <literal>PASSING</literal> clause.
do we need "The <replaceable>path_expression</replaceable>"?
also maybe we can add
+ In <literal>PASSING</literal> clause, <replaceable>varname</replaceable> is
+ the variables name, <replaceable>value</replaceable> is the
+ variables' value.
we can add a PASSING clause example from sqljson_queryfuncs.sql ,
since all three functions can use it.
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
+ <replaceable>context_item</replaceable> can be any character string that + can be succesfully cast to <type>jsonb</type>. typo: "succesfully", should be "successfully" 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>. + <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). + 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. Anyway, this is a minor issue.
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?
On Fri, Jul 5, 2024 at 10:16 PM Erik Rijkers <er@xs4all.nl> wrote:
> Op 7/5/24 om 14:35 schreef Amit Langote:
> > Hi Jian,
> >
> > Thanks for the reviews.
> >
> > [v3-0001-SQL-JSON-Various-improvements-to-SQL-JSON-query-f.patch]
> i.e., from the patch for doc/src/sgml/func.sgml
>
>
> Small changes:
>
> 4x:
> 'a SQL' should be
> 'an SQL'
> ('a SQL' does never occur in the docs; it's always 'an SQL'; apperently
> the 'sequel' pronunciation is out)
>
> 'some other type to which can be successfully coerced'
> 'some other type to which it can be successfully coerced'
>
>
> 'specifies the behavior behavior'
> 'specifies the behavior'
>
>
> In the following sentence:
>
> "By default, the result is returned as a value of type <type>jsonb</type>,
> though the <literal>RETURNING</literal> clause can be used to return
> the original <type>jsonb</type> value as some other type to which it
> can be successfully coerced."
>
> it seems to me that this phrase is better removed:
> "the original <type>jsonb</type> value as"
Thanks, I've addressed all these in the next patch I'll send.
--
Thanks, Amit Langote
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
Attachment
On Mon, Jul 8, 2024 at 8:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Updated patch attached.
>
Returns true if the SQL/JSON <replaceable>path_expression</replaceable>
- applied to the <replaceable>context_item</replaceable> using the
- <literal>PASSING</literal> <replaceable>value</replaceable>s yields any
- items.
+ applied to the <replaceable>context_item</replaceable> doesn't yield
+ any items.
should "doesn't" be removed?
should it be "yields"?
+ set. The <literal>ON ERROR</literal> clause specifies the behavior
+ if an error occurs when evaluating
<replaceable>path_expression</replaceable>,
+ when coercing the result value to the
<literal>RETURNING</literal> type,
+ or when evaluating the <literal>ON EMPTY</literal> expression if the
+ <replaceable>path_expression</replaceable> evaluation results in an
+ empty set.
last sentence, "in an empty set." should be "is an empty set"
Other than that, it looks good to me.
On Tue, Jul 9, 2024 at 10:39 AM jian he <jian.universality@gmail.com> wrote: > On Mon, Jul 8, 2024 at 8:57 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > Updated patch attached. > > > > Returns true if the SQL/JSON <replaceable>path_expression</replaceable> > - applied to the <replaceable>context_item</replaceable> using the > - <literal>PASSING</literal> <replaceable>value</replaceable>s yields any > - items. > + applied to the <replaceable>context_item</replaceable> doesn't yield > + any items. > should "doesn't" be removed? > should it be "yields"? Oops, fixed. > + set. The <literal>ON ERROR</literal> clause specifies the behavior > + if an error occurs when evaluating > <replaceable>path_expression</replaceable>, > + when coercing the result value to the > <literal>RETURNING</literal> type, > + or when evaluating the <literal>ON EMPTY</literal> expression if the > + <replaceable>path_expression</replaceable> evaluation results in an > + empty set. > last sentence, "in an empty set." should be "is an empty set" "results in an empty set" here means "the result of the evaluation is an empty set", similar to: $ git grep "results in an" doc doc/src/sgml/charset.sgml: results in an error, because even though the <literal>||</literal> operator doc/src/sgml/plpgsql.sgml: an omitted <literal>ELSE</literal> clause results in an error rather doc/src/sgml/plpython.sgml: If the second <literal>UPDATE</literal> statement results in an doc/src/sgml/pltcl.sgml: If the second <command>UPDATE</command> statement results in an Maybe I could just replace that by "returns an empty set". Will push shortly after making those changes. -- Thanks, Amit Langote
On Tue, Jul 9, 2024 at 12:30 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Jul 9, 2024 at 10:39 AM jian he <jian.universality@gmail.com> wrote: > > On Mon, Jul 8, 2024 at 8:57 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > > > Updated patch attached. > > > > > > > Returns true if the SQL/JSON <replaceable>path_expression</replaceable> > > - applied to the <replaceable>context_item</replaceable> using the > > - <literal>PASSING</literal> <replaceable>value</replaceable>s yields any > > - items. > > + applied to the <replaceable>context_item</replaceable> doesn't yield > > + any items. > > should "doesn't" be removed? > > should it be "yields"? > > Oops, fixed. > > > + set. The <literal>ON ERROR</literal> clause specifies the behavior > > + if an error occurs when evaluating > > <replaceable>path_expression</replaceable>, > > + when coercing the result value to the > > <literal>RETURNING</literal> type, > > + or when evaluating the <literal>ON EMPTY</literal> expression if the > > + <replaceable>path_expression</replaceable> evaluation results in an > > + empty set. > > last sentence, "in an empty set." should be "is an empty set" > > "results in an empty set" here means "the result of the evaluation is > an empty set", similar to: > > $ git grep "results in an" doc > doc/src/sgml/charset.sgml: results in an error, because even though > the <literal>||</literal> operator > doc/src/sgml/plpgsql.sgml: an omitted <literal>ELSE</literal> > clause results in an error rather > doc/src/sgml/plpython.sgml: If the second <literal>UPDATE</literal> > statement results in an > doc/src/sgml/pltcl.sgml: If the second <command>UPDATE</command> > statement results in an > > Maybe I could just replace that by "returns an empty set". > > Will push shortly after making those changes. And...pushed. Thanks for the reviews. -- Thanks, Amit Langote