Improve deparser expression handling, fix deparsing of DEFAULT expressions classified as "a_expr" #270
Conversation
We've removed them in previous refactorings, but didn't clean up the list.
This called the general deparseExpr function (a_expr), but should actually have called the deparseCExpr function (since AexprConst is part of c_expr). However we currently do not handle TypeCast nodes inside deparseCExpr, causing it to add parenthesis unnecessary. To mitigate, add a dedicated function for this.
This allows us to handle keyword-style FuncCalls correctly in deparseFuncCall, as will be done in a subsequent commit. Note that the safe choice here is to set the context to NONE, which intends to add parenthesis in places where a_expr is not allowed (b_expr or c_expr can be turned into an a_expr by being wrapped in parens)
This fixes a correctness issue where the deparsed SQL is not parsable, since the table DEFAULT expression is a b_expr, not an a_expr.
Note that the deparseAExpr function (used for deparsing A_Expr nodes) can be called from both a_expr and b_expr contexts, but we previously always passed NONE as the context. Besides now passing down the context, the function also used to add outer parenthesis to AEXPR_OP calls. This appears unnecessary (b_expr allows operator expressions without wrapping parenthesis) and was not utilized in practice, so remove that to avoid extra parenthesis.
There was a problem hiding this comment.
Thanks for this! Enjoyed reading through the commits and it helped me get a better sense of the flow through the deparser code. True, as a first-time committer I would not have landed a correct solution like this.
Not sure if it adds any information to your tests, but I added one example to the deparser test to explicitly support ALTER TABLE ... ADD COLUMN.
Co-authored-by: Tyler Gannon <[email protected]>
|
@tylergannon Thanks again for raising this - merged! FYI, we're currently planning to tag a libpg_query version with this (that also brings it to pg_query_go) sometime in the early new year, since there are a few other minor things we may take care of before the next release. My understanding is you have a working solution for your project in the interim, but let us know if it'd be important for you to get an earlier patch release. |
Alternate to #269 that fixes the discussed edge cases there as well.
Best reviewed by looking at individual commits.