Skip to content

perf: don't re-inline CSE'd expensive expressions in projection pushdown - #23459

Merged
xudong963 merged 3 commits into
apache:mainfrom
fordN:perf/cse-projection-pushdown
Jul 20, 2026
Merged

perf: don't re-inline CSE'd expensive expressions in projection pushdown#23459
xudong963 merged 3 commits into
apache:mainfrom
fordN:perf/cse-projection-pushdown

Conversation

@fordN

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow-up to #23395 (the volatile correctness fix, now merged), extending the same projection-pushdown guard to the performance case.

Rationale for this change

The logical CSE pass extracts a repeated scalar-function call (e.g. power(a, 2)) into a single intermediate projection referenced by column, so it is evaluated once per row. For file sources that can absorb computed projections (e.g. Parquet), the physical projection-pushdown rule then merges that projection into the scan's DataSourceExec, re-inlining the expression at every reference site and re-evaluating it N times per row — undoing the deduplication.

This is the performance sibling of #23220 (which fixed the correctness case for volatile expressions). It reproduces on file scans (Parquet/CSV), not in-memory tables.

EXPLAIN, beforepower(a, 2) evaluated 3× per row:

DataSourceExec: projection=[power(a,2)+b as x, power(a,2)-b as y, power(a,2)*c as z]

EXPLAIN, afterpower(a, 2) evaluated once, kept in a ProjectionExec:

ProjectionExec: [__common_expr_1+b as x, __common_expr_1-b as y, __common_expr_1*c as z]
DataSourceExec: projection=[power(a,2) as __common_expr_1, b, c]

What changes are included in this PR?

  • Extends the projection-pushdown guard in FileScanConfig::try_swapping_with_projection so it also declines the merge when it would duplicate a non-trivial expression. "Non-trivial" is decided by the existing expression-placement signal (KeepInPlace) — the same signal try_collapse_projection_chain uses — covering arithmetic, casts, and most scalar functions. Cheap leaf-pushable expressions (columns, get_field, input_file_name) still merge, so struct-field pushdown is unaffected. It reuses the volatile guard's multiplicity-aware reference counting, so an expression is blocked only when referenced more than once.
  • Adds the cse_projection_pushdown benchmark used to measure the change.

Are these changes tested?

Yes:

  • Unit tests in file_scan_config.rs: a non-trivial expr (arithmetic / scalar function) referenced ≥2 → blocked, once → allowed; a leaf-pushable scalar function (get_field) → allowed; volatile referenced ≥2 → blocked, once → allowed.
  • datafusion-sqllogictest suite passes. One existing plan in window.slt improves: a repeated c2 >= 2 comparison over a CSV scan is now computed once instead of being inlined twice into the DataSourceExec.
  • Benchmark A/B on cse_projection_pushdown (sample-size 50, 5s), with the no_repeated_exprs control confirming no change on unaffected queries:
BenchmarkChange
repeated_power (power(a,2) ×3)−40%
repeated_nested_fn (ln(abs(a)) ×3)−38%
repeated_sqrt (sqrt(a) ×3)−37%
mixed_repeated_unique−35%
repeated_cheap_abs (abs(a) ×3)within noise
no_repeated_exprs (control)no change

The gain scales with expression cost. For a single-instruction function like abs, caching the value in a ProjectionExec versus recomputing it is a wash (run-to-run noise), since the extra plan node roughly offsets the recomputation it saves.

Reproduce with:

cargo bench -p datafusion --bench cse_projection_pushdown --features parquet

Are there any user-facing changes?

No API changes. Queries that repeat an expensive expression over a file scan run faster; their physical plans retain a ProjectionExec above the scan (the expression evaluated once) instead of inlining it into the DataSourceExec projection.

@github-actionsgithub-actionsBot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) datasource Changes to the datasource crate labels Jul 10, 2026
@fordN

Copy link
Copy Markdown
ContributorAuthor

@xudong963 here's a follow up to #23395 that expands the set of expressions to not be "re-inline'd" to include performance considerations along with the correctness considerations.

In practice, this change has shown to have a significant effect on the performance of queries where outer statement references expensive computation results from inner.

@fordN
fordNforce-pushed the perf/cse-projection-pushdown branch from 80096e0 to 8b7250aCompareJuly 10, 2026 19:04
fordN added 2 commits July 14, 2026 10:52
Add a benchmark measuring queries that repeat a scalar function call, which the
logical CSE pass extracts into a single intermediate projection referenced by
column. It exercises the interaction between CSE and projection pushdown on
parquet sources; a follow-up will use it to demonstrate reducing redundant
re-evaluation of the extracted expression.
Extend the projection-pushdown guard so it also declines to merge a projection
into a file scan when the merge would duplicate a non-trivial expression (e.g.
`power(a, 2)`, `a + b`, `c2 >= 2`) that CSE extracted into a shared intermediate
projection. Without this the expression is re-inlined at every reference site
and re-evaluated per row, undoing the deduplication.
"Non-trivial" is decided by expression placement (`KeepInPlace`) — the same
signal `try_collapse_projection_chain` uses — so leaf-pushable expressions
(columns, `get_field`, `input_file_name`) still merge. References are counted
with multiplicity (reused from the volatile guard), so an expression is blocked
only when referenced more than once; volatile (correctness, apache#23220) and
non-trivial (performance) share the same threshold.
@fordN
fordNforce-pushed the perf/cse-projection-pushdown branch from 8b7250a to fea81f5CompareJuly 14, 2026 17:52

@xudong963xudong963 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about adding a small projection_pushdown.slt EXPLAIN test asserting:

ProjectionExec: ... __common_expr ...
DataSourceExec: projection=[power(a, 2) as __common_expr, ...]

Assert that a repeated `power(a, 2)` is kept as a single `ProjectionExec` above
the file scan (evaluated once) rather than inlined into the `DataSourceExec`
projection and re-evaluated per reference site. Requested in review.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@d1e6d45). Learn more about missing BASE report.

Additional details and impacted files
@@ Coverage Diff @@## main #23459 +/- ##
=======================================
Coverage ? 80.65% =======================================
Files ? 1086 Lines ? 366144 Branches ? 366144 =======================================
Hits ? 295318 Misses ? 53233 Partials ? 17593 

☔ 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.

@xudong963xudong963 added the performance Make DataFusion faster label Jul 17, 2026

@xudong963xudong963 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks again. I'll merge tomorrow this if no new comments

@xudong963
xudong963 added this pull request to the merge queueJul 20, 2026
Merged via the queue into apache:main with commit 8f15682Jul 20, 2026
38 checks passed
zhuqi-lucas added a commit to zhuqi-lucas/arrow-datafusion that referenced this pull request Jul 20, 2026
Previously the union-bounds fast path fired whenever
enable_hash_join_dynamic_membership_filter=false (the production
default). But that also relaxes the filter for pushdown_filters=true,
where arrow-rs RowFilter amortizes the per-row CASE hash-routed cost
via lazy decode of the remaining columns for filtered rows — the
tighter per-partition selectivity is worth paying for there.
This commit couples the fast path to the runtime pushdown_filters
setting: the SharedBuildAccumulator now mirrors
execution.parquet.pushdown_filters and picks the shape based on
where the filter will run:
* pushdown_filters=false → probe scan applies the filter post-decode
(PostScanFilter, Layer 3). Per-row cost dominates because there is
no lazy decode to amortize it. Emit the cheap union-of-bounds
predicate and skip membership entirely.
* pushdown_filters=true → probe scan applies the filter via arrow-rs
RowFilter (Layer 2), which skips the decode of the remaining
columns for rows that fail the predicate. Emit the historical
per-partition CASE hash-routed form; respect the user's opt-in via
enable_hash_join_dynamic_membership_filter for membership.
Effect matrix:
pushdown_filters | enable_membership_filter | Partitioned filter shape
-----------------+--------------------------+--------------------------------
false (default) | any | union bounds (fast path)
true | false | CASE hash-routed, bounds only
true | true | CASE hash-routed + membership
Tests:
- New helper make_partitioned_accumulator_pushdown_on_and_membership_{on,off}.
- New unit test partitioned_pushdown_on_membership_off_still_emits_case_no_membership
— pushdown=true must keep CASE even with membership off.
- New unit test collect_left_pushdown_off_skips_membership_even_with_gate_on
— pushdown=false must skip membership even with opt-in.
- Existing test fixtures updated to include pushdown_filters explicitly
(existing coverage kept — bounds-only fixtures set pushdown_filters=false
to match production default; membership-on fixtures set pushdown_filters=true).
Also refreshes projection_pushdown.slt for an unrelated main-branch
plan-shape change (apache#23459 CSE tweak) that surfaced post-rebase.
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
…own (apache#23459)
## Which issue does this PR close?
- Closesapache#23425.
Follow-up to apache#23395 (the volatile correctness fix, now merged),
extending the same projection-pushdown guard to the performance case.
## Rationale for this change
The logical CSE pass extracts a repeated scalar-function call (e.g.
`power(a, 2)`) into a single intermediate projection referenced by
column, so it is evaluated once per row. For file sources that can
absorb computed projections (e.g. Parquet), the physical
projection-pushdown rule then merges that projection into the scan's
`DataSourceExec`, re-inlining the expression at every reference site and
re-evaluating it N times per row — undoing the deduplication.
This is the performance sibling of apache#23220 (which fixed the *correctness*
case for volatile expressions). It reproduces on file scans
(Parquet/CSV), not in-memory tables.
**EXPLAIN, before** — `power(a, 2)` evaluated 3× per row:
```
DataSourceExec: projection=[power(a,2)+b as x, power(a,2)-b as y, power(a,2)*c as z]
```
**EXPLAIN, after** — `power(a, 2)` evaluated once, kept in a
`ProjectionExec`:
```
ProjectionExec: [__common_expr_1+b as x, __common_expr_1-b as y, __common_expr_1*c as z]
DataSourceExec: projection=[power(a,2) as __common_expr_1, b, c]
```
## What changes are included in this PR?
- Extends the projection-pushdown guard in
`FileScanConfig::try_swapping_with_projection` so it also declines the
merge when it would duplicate a **non-trivial** expression.
"Non-trivial" is decided by the existing expression-placement signal
(`KeepInPlace`) — the same signal `try_collapse_projection_chain` uses —
covering arithmetic, casts, and most scalar functions. Cheap
leaf-pushable expressions (columns, `get_field`, `input_file_name`)
still merge, so struct-field pushdown is unaffected. It reuses the
volatile guard's multiplicity-aware reference counting, so an expression
is blocked only when referenced more than once.
- Adds the `cse_projection_pushdown` benchmark used to measure the
change.
## Are these changes tested?
Yes:
- Unit tests in `file_scan_config.rs`: a non-trivial expr (arithmetic /
scalar function) referenced ≥2 → blocked, once → allowed; a
leaf-pushable scalar function (`get_field`) → allowed; volatile
referenced ≥2 → blocked, once → allowed.
- `datafusion-sqllogictest` suite passes. One existing plan in
`window.slt` improves: a repeated `c2 >= 2` comparison over a CSV scan
is now computed once instead of being inlined twice into the
`DataSourceExec`.
- Benchmark A/B on `cse_projection_pushdown` (sample-size 50, 5s), with
the `no_repeated_exprs` control confirming no change on unaffected
queries:
| Benchmark | Change |
|---|---|
| `repeated_power` (`power(a,2)` ×3) | −40% |
| `repeated_nested_fn` (`ln(abs(a))` ×3) | −38% |
| `repeated_sqrt` (`sqrt(a)` ×3) | −37% |
| `mixed_repeated_unique` | −35% |
| `repeated_cheap_abs` (`abs(a)` ×3) | within noise |
| `no_repeated_exprs` (control) | no change |
The gain scales with expression cost. For a single-instruction function
like `abs`, caching the value in a `ProjectionExec` versus recomputing
it is a wash (run-to-run noise), since the extra plan node roughly
offsets the recomputation it saves.
Reproduce with:
```
cargo bench -p datafusion --bench cse_projection_pushdown --features parquet
```
## Are there any user-facing changes?
No API changes. Queries that repeat an expensive expression over a file
scan run faster; their physical plans retain a `ProjectionExec` above
the scan (the expression evaluated once) instead of inlining it into the
`DataSourceExec` projection.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratedatasourceChanges to the datasource crateperformanceMake DataFusion fastersqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve CSE for expensive expressions when pushing projections into file scans

3 participants

@fordN@codecov-commenter@xudong963