Jah-yee commented on PR #21787: URL: https://github.com/apache/datafusion/pull/21787#issuecomment-4850787467
@kosiew Thank you for the review. Addressing both points: **1. Equal-precedence right-hand associativity (line 3289)**: Fixed Modified `write_child` to take an `is_right: bool` parameter. When `is_right && p == precedence`, the right child is wrapped in parentheses. This handles `a - (b - c)` (which would otherwise incorrectly render as `a - b - c`) and `a / (b / c)` correctly. Added test cases covering the right-assoc scenario: - `lit(1) - (lit(2) - lit(3))` → `1 - (2 - 3)` ✅ - `lit(1) / (lit(2) / lit(3))` → `1 / (2 / 3)` ✅ - Left chain still renders correctly: `(lit(1) - lit(2)) - lit(3)` → `1 - 2 - 3` ✅ **2. Code duplication with BinaryExpr Display (line 3279)**: Acknowledged You're right that both `BinaryExpr` `Display` and `SqlDisplay` now have similar precedence logic. Since this is a cleanup suggestion rather than a blocker, I'll add a `// TODO` in the `SqlDisplay` write_child noting the duplication for a follow-up refactor. The priority is getting the correctness fix merged first. Updated PR: https://github.com/apache/datafusion/pull/21787/commits Warmly, Jah-yee -- 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]
