Re: remaining sql/json patches

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.

> 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.

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