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