Always add parentheses when formatting BinaryExpr with SchemaDisplay
#16209
Conversation
hendrikmakait
changed the title
BinaryExprBinaryExpr with SchemaDisplay
|
Given how widely used this code is I suspect this change will be quite invasive / require changing many tests |
github-actions
bot
added
sql
github-actions
bot
added
the
documentation
| @@ -2500,7 +2500,7 @@ impl Display for SchemaDisplay<'_> { | |||
| } | |||
| } | |||
| Expr::BinaryExpr(BinaryExpr { left, op, right }) => { | |||
| write!(f, "{} {op} {}", SchemaDisplay(left), SchemaDisplay(right),) | |||
| write!(f, "({} {op} {})", SchemaDisplay(left), SchemaDisplay(right),) | |||
There was a problem hiding this comment.
This is the actual change.
|
@hendrikmakait Hi, there are still a few places that need to change |
hendrikmakait
changed the title
BinaryExpr with SchemaDisplayBinaryExpr with SchemaDisplay
|
@xudong963: CI is green now. |
cc @alamb for another look |
There was a problem hiding this comment.
Thanks @hendrikmakait and @xudong963 !
I agree this is better, but I see a bunch of tests where the parens are added even when they aren't necessary to distinguish order
Maybe we could add a check and only add parens to the display if they were needed (in some sort of Expr::Binary operator tree 🤔 )
| "| 2025-05-19 |", | ||
| "+-----------------------------------+", | ||
| "+-------------------------------------+", | ||
| "| make_date((t.y + Int64(1)),t.m,t.d) |", |
There was a problem hiding this comment.
The parens here seem unecessary (the top most expression) -- I wonder if we can avoid that somehow
|
@alamb: That's a fair point, I actually brought it up in the issue (#16054 (comment)). I'm happy to adjust the implementation. |
I think we should try to adjust if possible. Thank you @hendrikmakait |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
BinaryExprvia SchemaDisplayAre these changes tested?
Yes, added a regression test
Are there any user-facing changes?
No