Skip to content

fix: parenthesize nested expression display names - #23356

Open
goutamadwant wants to merge 8 commits into
apache:mainfrom
goutamadwant:fix-schema-display-parentheses
Open

fix: parenthesize nested expression display names#23356
goutamadwant wants to merge 8 commits into
apache:mainfrom
goutamadwant:fix-schema-display-parentheses

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Expression-derived display names could omit required parentheses for nested binary expressions. For example, (1 + 2) * 3 could be displayed as 1 + 2 * 3, which changes how the expression reads.

Unary expressions with binary children had the same ambiguity, such as NOT a = b and unary negative over a binary expression.

What changes are included in this PR?

  • Adds precedence-aware parenthesization for nested binary expressions in SchemaDisplay.
  • Applies the same parenthesization behavior to SqlDisplay.
  • Wraps binary children of unary NOT and unary negative expressions.
  • Adds focused unit coverage for (1 + 2) * 3, unary negative, and NOT with binary children.
  • Updates optimizer expected output where expression display names now include the corrected parentheses.

Are these changes tested?

  • cargo test -p datafusion-expr format_nested_binary_exprs_with_parentheses
  • cargo fmt --all
  • cargo test -p datafusion-expr
  • cargo test -p datafusion-optimizer
  • cargo clippy -p datafusion-expr -p datafusion-optimizer --all-targets --all-features -- -D warnings
  • git diff --check

Are there any user-facing changes?

Yes. Expression-derived column names and SQL-like expression display output now include parentheses when needed to preserve the intended expression structure.

@github-actions github-actions Bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Jul 7, 2026
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate labels Jul 7, 2026
@github-actions github-actions Bot added the sql SQL Planner label Aug 10, 2026
@codecov-commenter

codecov-commenter commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.35616% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.03%. Comparing base (66677fe) to head (13359f2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/expr/src/expr.rs 87.27% 1 Missing and 13 partials ⚠️
...tafusion/optimizer/src/common_subexpr_eliminate.rs 91.42% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #23356    +/-   ##
========================================
  Coverage   81.03%   81.03%            
========================================
  Files        1107     1107            
  Lines      384482   384615   +133     
  Branches   384482   384615   +133     
========================================
+ Hits       311563   311671   +108     
- Misses      54565    54572     +7     
- Partials    18354    18372    +18     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@goutamadwant

Copy link
Copy Markdown
Contributor Author

Fixed the SQLite sqllogictest failures by preserving each projection's original output name when common-subexpression elimination rewrites its expressions. This prevents the following optimize_projections pass from rebuilding a different schema name for parenthesized nested expressions.

@goutamadwant

Copy link
Copy Markdown
Contributor Author

The companion SQLite expectation update is now available in apache/datafusion-testing#22. Once it is merged, I’ll update this PR’s submodule pin to the resulting main commit.

With the companion update applied, the complete SQLite suite passes all 1,098 files using the CI-equivalent command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

2 participants