fix: don't duplicate volatile expressions when pushing projection into file scan by fordN · Pull Request #23395 · apache/datafusion

fordN · 2026-07-08T21:49:50Z

Which issue does this PR close?

Closes Projection pushdown into file scan duplicates non-deterministic functions (regression in 52.0.0) #23220.

Rationale for this change

The reported bug showed that when volatile (non-deterministic) expressions are referenced multiple times in an outer function the query results can be incorrect.

To produce correct results, a volatile expression (e.g. random(), uuid()) aliased once in a subquery and referenced multiple times must be evaluated once and reused. Since 52.0.0 this evaluation pattern for volatile functions has been broken; the physical projection-pushdown rule merges the outer projection into the file DataSourceExec, inlining the aliased volatile expression at each reference site. Instead of being evaluated once, the volatile expression is evaluated N times, so the references diverge:

SELECT s.r AS x, s.r AS y FROM (SELECT random() AS r FROM t) AS s;  -- x != y on >= 52.0.0

This was correct in 51.0.0 and regressed in 52.0.0/53.0.0. It reproduces on file scans (Parquet/CSV) but not in-memory tables, and was surfaced downstream in Ibis. Worth noting, #10337 appears to report the same class of bug but a different cause (I'll try to look into that issue soon as well).

What changes are included in this PR?

FileScanConfig::try_swapping_with_projection now declines to merge a projection into the file source when the merge would inline a volatile expression that the incoming projection references. The check reuses the existing is_volatile() utility from datafusion_physical_expr_common — the same volatility gate the physical ProjectionPushdown and FilterPushdown rules already apply — so file-source projection merging is now consistent with them. Deterministic expressions still merge freely.

The guard blocks when a volatile inner expression is referenced at least once (not only more than once), because a single outer expression can itself duplicate the value (e.g.r + r).

Are these changes tested?

Yes:

Unit tests in file_scan_config.rs:
- volatile referenced ≥1× → blocked;
- deterministic computed and column-only → allowed;
- unreferenced volatile → allowed;
- single-expression self-reference (r + r) → blocked;
- volatile nested in arithmetic → blocked;
- empty → allowed.
A regression test in projection_pushdown.slt asserting x = y for the aliased-random() pattern over a Parquet scan.
Full datafusion-sqllogictest suite passes.

Are there any user-facing changes?

No API changes! Query results for the affected pattern are corrected (a volatile expression aliased in a subquery is no longer duplicated by projection pushdown into a file scan). Physical plans for such queries now retain a ProjectionExec above the scan rather than inlining the volatile expression into the DataSourceExec projection.

fordN · 2026-07-08T23:05:40Z

First-time contributor here! So I'll need a committer to approve the CI workflow.
cc. @alamb since you were involved in related discussion on #12599

xudong963 · 2026-07-09T05:22:12Z

+    let mut referenced = vec![false; inner_exprs.len()];
+    for proj_expr in outer_exprs {
+        for col in collect_columns(&proj_expr.expr) {
+            let idx = col.index();
+            if idx < referenced.len() {
+                referenced[idx] = true;
+            }
+        }
+    }
+
+    referenced
+        .iter()
+        .enumerate()
+        .any(|(idx, &referenced)| referenced && is_volatile(&inner_exprs[idx].expr))


That is safe but imprecise. A better implementation would count references with expr.apply(...), like try_collapse_projection_chain already does and block only when a volatile inner expression is referenced more than once.

@xudong963 good call. I've updated the reference counting algorithm to count references with multiplicity similar to how it's done in try_collapse_projection_chain(). Now with more precise counting we can block only when volatile expression is referenced more than once.

I've also added a new unit test for the single-reference case.

…o file scan A volatile expression (e.g. `random()`, `uuid()`) aliased once in a subquery and referenced multiple times must be evaluated once and reused. The physical projection-pushdown rule merged the outer projection into the file `DataSourceExec`, inlining the aliased volatile expression at every reference site and turning one call into N independent calls, so the references diverged. This regressed in 52.0.0 and reproduces on file scans (parquet/csv) but not in-memory tables. Guard `FileScanConfig::try_swapping_with_projection`: decline the merge when a volatile expression in the scan's existing projection is referenced by the incoming projection, reusing the existing `is_volatile()` utility -- the same gate the physical `ProjectionPushdown` and `FilterPushdown` rules already apply. Deterministic expressions still merge freely. Closes apache#23220.

github-actions · 2026-07-10T01:37:46Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-datasource v54.0.0 (current)
       Built [  38.702s] (current)
     Parsing datafusion-datasource v54.0.0 (current)
      Parsed [   0.030s] (current)
    Building datafusion-datasource v54.0.0 (baseline)
       Built [  36.774s] (baseline)
     Parsing datafusion-datasource v54.0.0 (baseline)
      Parsed [   0.032s] (baseline)
    Checking datafusion-datasource v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.252s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/function_missing.ron

Failed in:
  function datafusion_datasource::file_scan_config::output_partitioning_from_partition_fields, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/be6f5df98a10c764b1979e225f22ca386122c7c5/datafusion/datasource/src/file_scan_config/mod.rs:582

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  77.023s] datafusion-datasource
    Building datafusion-sqllogictest v54.0.0 (current)
       Built [ 182.876s] (current)
     Parsing datafusion-sqllogictest v54.0.0 (current)
      Parsed [   0.021s] (current)
    Building datafusion-sqllogictest v54.0.0 (baseline)
       Built [ 177.463s] (baseline)
     Parsing datafusion-sqllogictest v54.0.0 (baseline)
      Parsed [   0.024s] (baseline)
    Checking datafusion-sqllogictest v54.0.0 -> v54.0.0 (no change; assume patch)
     Checked [   0.092s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 363.172s] datafusion-sqllogictest

xudong963 · 2026-07-10T08:08:27Z

@fordN Thanks for the first contribution!

…o file scan (apache#23395) ## Which issue does this PR close? - Closes apache#23220. ## Rationale for this change The reported bug showed that when volatile (non-deterministic) expressions are referenced multiple times in an outer function the query results can be incorrect. To produce correct results, a volatile expression (e.g. `random()`, `uuid()`) aliased once in a subquery and referenced multiple times must be evaluated once and reused. Since 52.0.0 this evaluation pattern for volatile functions has been broken; the physical projection-pushdown rule merges the outer projection into the file `DataSourceExec`, inlining the aliased volatile expression at each reference site. Instead of being evaluated once, the volatile expression is evaluated N times, so the references diverge: ```sql SELECT s.r AS x, s.r AS y FROM (SELECT random() AS r FROM t) AS s; -- x != y on >= 52.0.0 ``` This was correct in 51.0.0 and regressed in 52.0.0/53.0.0. It reproduces on file scans (Parquet/CSV) but not in-memory tables, and was surfaced downstream in Ibis. Worth noting, apache#10337 appears to report the same class of bug but a different cause (I'll try to look into that issue soon as well). ## What changes are included in this PR? `FileScanConfig::try_swapping_with_projection` now declines to merge a projection into the file source when the merge would inline a volatile expression that the incoming projection references. The check reuses the existing `is_volatile()` utility from `datafusion_physical_expr_common` — the same volatility gate the physical `ProjectionPushdown` and `FilterPushdown` rules already apply — so file-source projection merging is now consistent with them. Deterministic expressions still merge freely. The guard blocks when a volatile inner expression is referenced at least once (not only more than once), because a single outer expression can itself duplicate the value (e.g.`r + r`). ## Are these changes tested? Yes: - Unit tests in `file_scan_config.rs`: - volatile referenced ≥1× → blocked; - deterministic computed and column-only → allowed; - unreferenced volatile → allowed; - single-expression self-reference (`r + r`) → blocked; - volatile nested in arithmetic → blocked; - empty → allowed. - A regression test in `projection_pushdown.slt` asserting `x = y` for the aliased-`random()` pattern over a Parquet scan. - Full `datafusion-sqllogictest` suite passes. ## Are there any user-facing changes? No API changes! Query results for the affected pattern are corrected (a volatile expression aliased in a subquery is no longer duplicated by projection pushdown into a file scan). Physical plans for such queries now retain a `ProjectionExec` above the scan rather than inlining the volatile expression into the `DataSourceExec` projection.

…ojection into file scan (backport #23395, adapted) (#23585) ## Which issue does this PR close? - Backport of #23395 to branch-54 (#22547) - Closes #23220. ## Rationale for this change The reported bug showed that when volatile (non-deterministic) expressions are referenced multiple times in an outer function the query results can be incorrect. To produce correct results, a volatile expression (e.g. `random()`, `uuid()`) aliased once in a subquery and referenced multiple times must be evaluated once and reused. Since 52.0.0 this evaluation pattern for volatile functions has been broken; the physical projection-pushdown rule merges the outer projection into the file `DataSourceExec`, inlining the aliased volatile expression at each reference site. Instead of being evaluated once, the volatile expression is evaluated N times, so the references diverge: ```sql SELECT s.r AS x, s.r AS y FROM (SELECT random() AS r FROM t) AS s; -- x != y on >= 52.0.0 ``` This was correct in 51.0.0 and regressed in 52.0.0/53.0.0. It reproduces on file scans (Parquet/CSV) but not in-memory tables, and was surfaced downstream in Ibis. Worth noting, #10337 appears to report the same class of bug but a different cause (I'll try to look into that issue soon as well). ## What changes are included in this PR? `FileScanConfig::try_swapping_with_projection` now declines to merge a projection into the file source when the merge would inline a volatile expression that the incoming projection references. The check reuses the existing `is_volatile()` utility from `datafusion_physical_expr_common` — the same volatility gate the physical `ProjectionPushdown` and `FilterPushdown` rules already apply — so file-source projection merging is now consistent with them. Deterministic expressions still merge freely. The guard blocks when a volatile inner expression is referenced at least once (not only more than once), because a single outer expression can itself duplicate the value (e.g.`r + r`). ## Are these changes tested? Yes: - Unit tests in `file_scan_config.rs`: - volatile referenced ≥1× → blocked; - deterministic computed and column-only → allowed; - unreferenced volatile → allowed; - single-expression self-reference (`r + r`) → blocked; - volatile nested in arithmetic → blocked; - empty → allowed. - A regression test in `projection_pushdown.slt` asserting `x = y` for the aliased-`random()` pattern over a Parquet scan. - Full `datafusion-sqllogictest` suite passes. ## Are there any user-facing changes? No API changes! Query results for the affected pattern are corrected (a volatile expression aliased in a subquery is no longer duplicated by projection pushdown into a file scan). Physical plans for such queries now retain a `ProjectionExec` above the scan rather than inlining the volatile expression into the `DataSourceExec` projection. Co-authored-by: Matt Butrovich <mbutrovich@users.noreply.github.com>

…own (apache#23459) ## Which issue does this PR close? - Closes apache#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.

) ## Which issue does this PR close? - Part of apache#25459. It does not fix a bug. It adds a test helper for the class of bug where the optimizer evaluates a volatile call more times than the query says. ## Rationale for this change Tests for these bugs used `random()` and inferred the number of evaluations from the result: they compared two reads of the same value, ran 10000–100000 rows and checked that no row broke a filter, or wrapped arguments in `case when random() >= 0 then x end`. These tests are indirect, and a reader cannot see what they check. A function that counts its own evaluations makes them exact. An earlier version of this PR added a check that walked the optimized plan. Run on every query of the sqllogictest suite with the bugs reverted, it caught none of them, and it could not see duplication in the physical plan. This version replaces it. ## What changes are included in this PR? Three functions, registered in every sqllogictest context (`datafusion/sqllogictest/src/test_context/evaluation_counter.rs`): - `counted_volatile(x)` returns `x` and is volatile, like `random()` - `counted_immutable(x)` returns `x` and is immutable - `evaluations()` returns the number of rows that the two functions above were evaluated for since its last call, and resets the count ``` query I SELECT count(*) FROM (SELECT counted_volatile(a) AS r FROM t) WHERE r > 5; ---- 5 query I SELECT evaluations(); ---- 10 ``` The count is per row, not per call, so it does not depend on batch size, and it is correct for `CASE` and `COALESCE`, which evaluate a branch on part of a batch. Commits, one per step: 1. Add the functions. 2. Port the tests for apache#25415 (`push_down_filter_regression.slt`). 3. Port the tests for apache#23220 and apache#24678 (`projection_pushdown.slt`). 4. Port the tests for apache#25477 (`coalesce.slt`). 5. Port the tests that two volatile calls are not merged (`functions.slt`, `aggregates_simplify.slt`). 6. Add `volatile_evaluations.slt`: filter pushdown, joins, unions, `HAVING`, and two known double evaluations: apache#26910 (preimage, a bug) and apache#25457 (`BETWEEN`, accepted behaviour). Not ported: `cse.slt` (its `EXPLAIN` tests check which subexpressions are hoisted, not only how many evaluations), `cte_materialized.slt` (its results are already exact), and the files that use `random()` only as an expression that cannot be pushed down. ## Are these changes tested? Each ported test was checked against its fix reverted: | Fix | Count with the fix reverted | |---|---| | apache#25416 (volatile group key) | 13 and 17, expected 10 | | apache#24720 (leaf extraction) | 20, expected 10 | | apache#23395 (file scan projection, physical plan) | 6, expected 3 | | apache#25561 (`COALESCE`) | 30, expected 20 | ## Are there any user-facing changes? No. The functions exist only in the sqllogictest runner. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jay Zhan <jayzhan211@gmail.com>

github-actions Bot added sqllogictest SQL Logic Tests (.slt) datasource Changes to the datasource crate labels Jul 8, 2026

xudong963 approved these changes Jul 9, 2026

View reviewed changes

fordN force-pushed the fix/projection-pushdown-volatile-duplication branch 2 times, most recently from 8116cc7 to d4cdf6f Compare July 9, 2026 16:32

fordN mentioned this pull request Jul 9, 2026

Preserve CSE for expensive expressions when pushing projections into file scans #23425

Closed

fordN force-pushed the fix/projection-pushdown-volatile-duplication branch 2 times, most recently from 6f497eb to b5c72c4 Compare July 9, 2026 18:14

fordN force-pushed the fix/projection-pushdown-volatile-duplication branch from b5c72c4 to a32fd97 Compare July 9, 2026 18:19

github-actions Bot added the auto detected api change Auto detected API change label Jul 10, 2026

xudong963 removed the auto detected api change Auto detected API change label Jul 10, 2026

xudong963 added this pull request to the merge queue Jul 10, 2026

Merged via the queue into apache:main with commit 1e77af8 Jul 10, 2026
36 checks passed

discord9 mentioned this pull request Jul 15, 2026

fix(backport): avoid duplicating volatile file projections GreptimeTeam/datafusion#17

Open

discord9 mentioned this pull request Jul 20, 2026

fix: port GreptimeDB correctness fixes to DataFusion 54 GreptimeTeam/datafusion#20

Closed

nuno-faria added the v55.0.0 label Oct 3, 2026

adriangb mentioned this pull request Oct 30, 2026

test: count evaluations of volatile calls in sqllogictests #25458

Merged

Navigation Menu

Repository navigation

fix: don't duplicate volatile expressions when pushing projection into file scan - #23395

fix: don't duplicate volatile expressions when pushing projection into file scan#23395
xudong963 merged 1 commit into
apache:mainfrom
fordN:fix/projection-pushdown-volatile-duplication

fordN commented Jul 8, 2026

Uh oh!

fordN commented Jul 8, 2026

Uh oh!

xudong963 Jul 9, 2026

Uh oh!

fordN Jul 9, 2026

Uh oh!

github-actions Bot commented Jul 10, 2026

Uh oh!

xudong963 commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fordN commented Jul 8, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

fordN commented Jul 8, 2026

Uh oh!

xudong963 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

fordN Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 10, 2026

Uh oh!

xudong963 commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants