kosiew commented on code in PR #21787:
URL: https://github.com/apache/datafusion/pull/21787#discussion_r3172648998
##########
datafusion/expr/src/expr.rs:
##########
@@ -3304,7 +3304,31 @@ impl Display for SqlDisplay<'_> {
}
}
Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
- write!(f, "{} {op} {}", SqlDisplay(left), SqlDisplay(right),)
+ // Add parentheses around child binary expressions to
correctly reflect
+ // precedence. Same logic as BinaryExpr Display impl.
+ fn write_child(
+ f: &mut Formatter<'_>,
+ expr: &Expr,
+ precedence: u8,
+ ) -> fmt::Result {
+ match expr {
+ Expr::BinaryExpr(child) => {
+ let p = child.op.precedence();
+ if p == 0 || p < precedence {
Review Comment:
I think this still needs to account for equal-precedence expressions on the
right-hand side.
For example, `lit(1) - (lit(2) - lit(3))` would currently render as `1 - 2 -
3`, which is normally read as `(1 - 2) - 3`. Similarly, `a / (b * c)` would
render as `a / b * c`.
I would expect `human_display()` to preserve the expression tree semantics
whenever it omits parentheses. Could we distinguish left and right children
here, and parenthesize right-hand children with equal precedence when the
parent and child operator combination is not safely associative?
##########
datafusion/expr/src/expr.rs:
##########
@@ -3304,7 +3304,31 @@ impl Display for SqlDisplay<'_> {
}
}
Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
- write!(f, "{} {op} {}", SqlDisplay(left), SqlDisplay(right),)
+ // Add parentheses around child binary expressions to
correctly reflect
+ // precedence. Same logic as BinaryExpr Display impl.
Review Comment:
This looks like it duplicates the existing `BinaryExpr` `Display` precedence
logic.
Once the associativity and equal-precedence case is fixed, could we extract
a small shared helper, perhaps parameterized by the child display function, so
`Display` and `human_display()` do not drift again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
