Re: remaining sql/json patches

On Thu, Jul 20, 2023 at 17:19 Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> On Wed, Jul 19, 2023 at 5:17 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> wrote:
> > > On 2023-Jul-18, Amit Langote wrote:
> > >
> > > > Attached updated patches. In 0002, I removed the mention of the
> > > > RETURNING clause in the JSON(), JSON_SCALAR() documentation, which I
> > > > had forgotten to do in the last version which removed its support in
> > > > code.
> > >
> > > > I think 0001 looks ready to go. Alvaro?
> > >
> > > It looks reasonable to me.
> >
> > Thanks for taking another look.
> >
> > I will push this tomorrow.
>
> Pushed.
>
> > > > Also, I've been wondering if it isn't too late to apply the following
> > > > to v16 too, so as to make the code look similar in both branches:
> > >
> > > Hmm.
> > >
> > > > 785480c953 Pass constructName to transformJsonValueExpr()
> > >
> > > I think 785480c953 can easily be considered a bugfix on 7081ac46ace8,
> so
> > > I agree it's better to apply it to 16.
> >
> > OK.
>
> Pushed to 16.
>
> > > > b6e1157e7d Don't include CaseTestExpr in JsonValueExpr.formatted_expr
> > >
> > > I feel a bit uneasy about this one. It seems to assume that
> > > formatted_expr is always set, but at the same time it's not obvious
> that
> > > it is. (Maybe this aspect just needs some more commentary).
> >
> > Hmm, I agree that the comments about formatted_expr could be improved
> > further, for which I propose the attached. Actually, staring some
> > more at this, I'm inclined to change makeJsonValueExpr() to allow
> > callers to pass it the finished 'formatted_expr' rather than set it by
> > themselves.
> >
> > > I agree
> > > that it would be better to make both branches identical, because if
> > > there's a problem, we are better equipped to get a fix done to both.
> > >
> > > As for the removal of makeCaseTestExpr(), I agree -- of the six callers
> > > of makeNode(CastTestExpr), only two of them would be able to use the
> new
> > > function, so it doesn't look of general enough usefulness.
> >
> > OK, so you agree with back-patching this one too, though perhaps only
> > after applying something like the aforementioned patch.
>
> I looked at this some more and concluded that it's fine to think that
> all JsonValueExpr nodes leaving the parser have their formatted_expr
> set. I've updated the commentary some more in the patch attached as
> 0001.
>
> Rebased SQL/JSON patches also attached. I've fixed the JSON_TABLE
> syntax synopsis in the documentation as mentioned in my other email.

I’m thinking of pushing 0001 and 0002 tomorrow barring objections.

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