Re: SQL/JSON query functions context_item doc entry and type requirement

On Thu, Jun 27, 2024 at 9:01 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Sat, Jun 22, 2024 at 6:39 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangote09(at)gmail(dot)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