fix: don't duplicate volatile expressions when pushing projection into file scan#23395
Conversation
| 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
8116cc7 to
d4cdf6f
Compare
6f497eb to
b5c72c4
Compare
…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.
b5c72c4 to
a32fd97
Compare
|
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 |
|
@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?
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 fileDataSourceExec, 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: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_projectionnow 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 existingis_volatile()utility fromdatafusion_physical_expr_common— the same volatility gate the physicalProjectionPushdownandFilterPushdownrules 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:
file_scan_config.rs:r + r) → blocked;projection_pushdown.sltassertingx = yfor the aliased-random()pattern over a Parquet scan.datafusion-sqllogictestsuite 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
ProjectionExecabove the scan rather than inlining the volatile expression into theDataSourceExecprojection.