Skip to content

Conversation

@AndreaBozzo
Copy link

Which issue does this PR close?

Closes #16054

Rationale for this change

Binary expressions like (1+2)*3 were displayed incorrectly as Int64(1) + Int64(2) * Int64(3) without parentheses, which is misleading because it doesn't preserve the operator precedence from the original expression.

What changes are included in this PR?

  • Modified Display implementation for BinaryExpr to always wrap nested binary expressions in parentheses
  • Applied the same fix to SchemaDisplay and SqlDisplay wrappers
  • Added tests for binary expression display with various precedence scenarios

How are these changes tested?

  • New unit test test_binary_expr_display_with_parentheses covering:
    • Arithmetic expressions: (1+2)*3
    • Logical expressions: (a OR b) AND c
  • Existing tests pass

Are these changes safe?

Yes. This follows DuckDB's approach of always adding parentheses around nested binary expressions, which was discussed and approved in the issue comments.

Example

Before:

> select (1+2)*3;
+--------------------------------+
| Int64(1) + Int64(2) * Int64(3) |
+--------------------------------+

After:

> select (1+2)*3;
+------------------------------------+
| (Int64(1) + Int64(2)) * Int64(3)   |
+------------------------------------+

Always wrap nested binary expressions in parentheses when formatting
to avoid ambiguity. For example, `(1+2)*3` now displays as
`(Int64(1) + Int64(2)) * Int64(3)` instead of the misleading
`Int64(1) + Int64(2) * Int64(3)`.

This follows DuckDB's approach of always adding parentheses around
nested binary expressions.

Fixes apache#16054
@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jan 20, 2026
@github-actions github-actions bot added the core Core DataFusion crate label Jan 21, 2026
Updates all test snapshots affected by the binary expression Display
parentheses change.

The `test_pretty_roundtrip` test had a flawed assertion that checked
structural equality after round-tripping through the pretty unparser.
This was incorrect because the pretty unparser intentionally removes
"unnecessary" parentheses, which can change expression associativity:
- Input: `(id + 5) * (age * 8)` (right-grouped multiplication)
- Pretty output: `(id + 5) * age * 8`
- Re-parsed: `((id + 5) * age) * 8` (left-grouped due to associativity)

These are semantically equivalent for associative operations but
structurally different, so the round-trip assertion was removed.
@github-actions github-actions bot added sql SQL Planner optimizer Optimizer rules substrait Changes to the substrait crate labels Jan 21, 2026
Copy link
Author

@AndreaBozzo AndreaBozzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i crashed locally while 14f4b0b , added a new commit with the rest of snapshots

edit: 4 more to update

additional edits: i had to update many sqllogictests, i have been helped by ai in this

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expr formatting missing parentheses

1 participant