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.
I’m thinking of pushing 0001 and 0002 tomorrow barring objections.
