Re: remaining sql/json patches

On Wed, Jul 12, 2023 at 10:23 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Jul 12, 2023 at 6:41 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Mon, Jul 10, 2023 at 11:52 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > > I forgot to add:
> >
> > Thanks for the review of these.
> >
> > > * 0001 looks an obvious improvement. You could just push it now, to
> > > avoid carrying it forward anymore. I would just put the constructName
> > > ahead of value expr in the argument list, though.
> >
> > Sure, that makes sense.
> >
> > > * 0002: I have no idea what this is (though I probably should). I would
> > > also push it right away -- if anything, so that we figure out sooner
> > > that it was actually needed in the first place. Or maybe you just need
> > > the right test cases?
> >
> > Hmm, I don't think having or not having CaseTestExpr makes a
> > difference to the result of evaluating JsonValueExpr.format_expr, so
> > there are no test cases to prove one way or the other.
> >
> > After staring at this again for a while, I think I figured out why the
> > CaseTestExpr might have been put there in the first place. It seems
> > to have to do with the fact that JsonValueExpr.raw_expr is currently
> > evaluated independently of JsonValueExpr.formatted_expr and the
> > CaseTestExpr propagates the result of the former to the evaluation of
> > the latter. Actually, formatted_expr is effectively
> > formatting_function(<result-of-raw_expr>), so if we put raw_expr
> > itself into formatted_expr such that it is evaluated as part of
> > evaluating formatted_expr, then there is no need for the CaseTestExpr
> > as the propagator for raw_expr's result.
> >
> > I've expanded the commit message to mention the details.
> >
> > I'll push these tomorrow.
>
> I updated it to make the code in makeJsonConstructorExpr() that *does*
> need to use a CaseTestExpr a bit more readable. Also, updated the
> comment above CaseTestExpr to mention this instance of its usage.

Pushed these two just now.

> > On Mon, Jul 10, 2023 at 11:47 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > > On 2023-Jul-10, Amit Langote wrote:
> > > > > I'm not in love with the fact that JSON and JSONB have pretty much
> > > > > parallel type categorizing functionality. It seems entirely artificial.
> > > > > Maybe this didn't matter when these were contained inside each .c file
> > > > > and nobody else had to deal with that, but I think it's not good to make
> > > > > this an exported concept. Is it possible to do away with that? I mean,
> > > > > reduce both to a single categorization enum, and a single categorization
> > > > > API. Here you have to cast the enum value to int in order to make
> > > > > ExecInitExprRec work, and that seems a bit lame; moreso when the
> > > > > "is_jsonb" is determined separately (cf. ExecEvalJsonConstructor)
> > > >
> > > > OK, I agree that a unified categorizing API might be better. I'll
> > > > look at making this better. Btw, does src/include/common/jsonapi.h
> > > > look like an appropriate place for that?
> > >
> > > Hmm, that header is frontend-available, and the type-category appears to
> > > be backend-only, so maybe no. Perhaps jsonfuncs.h is more apropos?
> > > execExpr.c is already dealing with array internals, so having to deal
> > > with json internals doesn't seem completely out of place.
> >
> > OK, attached 0003 does it like that. Essentially, I decided to only
> > keep JsonTypeCategory and json_categorize_type(), with some
> > modifications to accommodate the callers in jsonb.c.
> >
> > > > > In the 2023 standard, JSON_SCALAR is just
> > > > >
> > > > > <JSON scalar> ::= JSON_SCALAR <left paren> <value expression> <right paren>
> > > > >
> > > > > but we seem to have added a <JSON output format> clause to it. Should
> > > > > we really?
> > > >
> > > > Hmm, I am not seeing <JSON output format> in the rule for JSON_SCALAR,
> > >
> > > Agh, yeah, I confused myself, sorry.
> > >
> > > > Per what I wrote above, the grammar for JSON() and JSON_SCALAR() does
> > > > not allow specifying the FORMAT clause. Though considering what you
> > > > wrote, the RETURNING clause does appear to be an extension to the
> > > > standard's spec.
> > >
> > > Hmm, I see that <JSON output clause> (which is RETURNING plus optional
> > > FORMAT) appears included in JSON_OBJECT, JSON_ARRAY, JSON_QUERY,
> > > JSON_SERIALIZE, JSON_OBJECTAGG, JSON_ARRAYAGG. It's not necessarily a
> > > bad thing to have it in other places, but we should consider it
> > > carefully. Do we really want/need it in JSON() and JSON_SCALAR()?
> >
> > I thought that removing that support breaks JSON_TABLE() or something
> > but it doesn't, so maybe we can do without the extension if there's no
> > particular reason it's there in the first place. Maybe Andrew (cc'd)
> > remembers why he decided in [1] to (re-) add the RETURNING clause to
> > JSON() and JSON_SCALAR()?
> >
> > Updated patches, with 0003 being a new refactoring patch, are
> > attached. Patches 0004~ contain a few updates around JsonValueExpr.
> > Specifically, I removed the case for T_JsonValueExpr in
> > transformExprRecurse(), because I realized that JsonValueExpr
> > expressions never appear embedded in other expressions. That allowed
> > me to get rid of some needless refactoring around
> > transformJsonValueExpr() in the patch that adds JSON_VALUE() etc.
>
> I noticed that 0003 was giving some warnings, which is fixed in the
> attached updated set of patches.

Here are the remaining patches, rebased. I'll remove the RETURNING
clause from JSON() and JSON_SCALAR() in the next version that I will
post tomorrow unless I hear objections.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com