On Thu, Jun 27, 2024 at 9:01 PM Amit Langote <amitlangot...@gmail.com> wrote:
> On Sat, Jun 22, 2024 at 6:39 PM jian he <jian.universal...@gmail.com> wrote:
> > On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangot...@gmail.com>
> > wrote:
> > >
> > > >> JSON_VALUE on error, on empty semantics should be the same as
> > > >> json_query.
> > > >> like:
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON EMPTY ]
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON ERROR ])
> > > >>
> > > >> examples:
> > > >> select JSON_value(jsonb '[]' , '$' empty array on error);
> > > >> select JSON_value(jsonb '[]' , '$' empty object on error);
> > > >
> > > > Again the documented behavior seems to make sense though and the
> > > > ability to specify empty in the value function seems like a bug. If
> > > > you really want an empty array or object you do have access to default.
> > > > The reason json_query provides for an empty array/object is that it is
> > > > already expecting to produce an array (object seems a little odd).
> > > >
> > > > I agree our docs and code do not match which needs to be fixed, ideally
> > > > in the direction of the standard which I'm guessing our documentation
> > > > is based off of. But let's not go off of my guess.
> > >
> > > Oops, that is indeed not great and, yes, the problem is code not
> > > matching the documentation, the latter of which is "correct".
> > >
> > > Basically, the grammar allows specifying any of the all possible ON
> > > ERROR/EMPTY behavior values irrespective of the function, so parse
> > > analysis should be catching and flagging an attempt to specify
> > > incompatible value for a given function, which it does not.
> > >
> > > I've attached a patch to fix that, which I'd like to push before
> > > anything else we've been discussing.
> > >
> >
> > + errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("invalid ON ERROR behavior"),
> > + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> > <value> is allowed in ON ERROR for JSON_QUERY()."),
> > + parser_errposition(pstate, func->on_error->location));
> >
> > `EMPTY [ ARRAY | OBJECT }` seems not correct,
> > maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> > (apply to other places)
>
> Or EMPTY [ ARRAY ], EMPTY OBJECT
>
> > `DEFAULT <value>`
> > `DEFAULT <expression>` or just `DEFAULT expression` would be more correct?
> > (apply to other places)
>
> "DEFAULT expression" sounds good.
>
> > I think we should make json_query, json_value on empty, on error
> > behave the same way.
> > otherwise, it will have consistency issues for scalar jsonb.
> > for example, we should expect the following two queries to return the
> > same result?
> > SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> > SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
> >
> > Also the json_table function will call json_value or json_query,
> > make these two functions on error, on empty behavior the same can
> > reduce unintended complexity.
> >
> > So based on your
> > patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> > and the above points, I have made some changes, attached.
> > it will make json_value, json_query not allow {true | false | unknown
> > } on error, {true | false | unknown } on empty.
> > json_table error message deal the same way as
> > b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
>
> Here is an updated patch that I think takes care of these points.
>
> > BTW,
> > i found one JSON_TABLE document deficiency
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON ERROR ]
> >
> > it should be
> >
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON ERROR ]
>
> You're right. Fixed.
>
> Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
> columns which is meaningless because JSON_EXISTS() doesn't have a
> corresponding ON EMPTY clause. Fixed grammar to prevent that in the
> attached 0002.
I've pushed this for now to close out the open item.
I know there's some documentation improvement work left to do [1],
which I'll try to find some time for next week.
--
Thanks, Amit Langote
[1]
https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com
