fix: add parentheses in binary expr human_display to reflect precedence#21787
fix: add parentheses in binary expr human_display to reflect precedence#21787Jah-yee wants to merge 2 commits intoapache:mainfrom
Conversation
Fixes apache#16054 - Expr formatting missing parentheses The human_display() method for BinaryExpr was not adding parentheses to correctly reflect operator precedence. For example, (1+2)*3 was displayed as 1 + 2 * 3 instead of (1 + 2) * 3. This change adds a write_child helper that checks operator precedence and wraps child BinaryExprs in parentheses when the child has lower precedence than the parent. Added test_human_display_binary_expr_parens to verify the fix.
|
I think we'd also need to fix |
| match expr { | ||
| Expr::BinaryExpr(child) => { | ||
| let p = child.op.precedence(); | ||
| if p == 0 || p < precedence { |
There was a problem hiding this 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?
| 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. |
There was a problem hiding this 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?
4 participants
Good day,
This PR fixes #16054 - "Expr formatting missing parentheses".
Problem
The
human_display()method forBinaryExprwas not adding parentheses to correctly reflect operator precedence. For example,(1+2)*3was displayed as1 + 2 * 3instead of(1 + 2) * 3.Solution
Added a
write_childhelper function that checks operator precedence and wraps childBinaryExprs in parentheses when the child has lower precedence than the parent. This matches the behavior in theBinaryExprDisplayimpl.Changes
SqlDisplay::writeforExpr::BinaryExprto use precedence-aware child formattingtest_human_display_binary_expr_parenstest to verify the fixTest results
cargo test -p datafusion-expr test_human_display_binary_expr_parenspasses ✅Verification
Thank you for your attention. If there are any issues or suggestions, please leave a comment and I will address them promptly.
Warmly, Jah-yee