On Wed, Jul 19, 2023 at 5:17 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Jul 19, 2023 at 12:53 AM Alvaro Herrera <alvherre@alvh.no-ip.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.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com