Hi,
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.
--
Thanks, Amit Langote
v2-0002-SQL-JSON-Prevent-ON-EMPTY-for-EXISTS-columns-in-J.patch
Description: Binary data
v2-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch
Description: Binary data
