You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jaideeppyne (via GitHub)" <gi...@apache.org> on 2026/08/27 11:07:06 UTC
[PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
jaideeppyne opened a new pull request, #24720:
URL: https://github.com/apache/datafusion/pull/24720
## Which issue does this PR close?
- Closes #24678.
## Rationale for this change
A column alias denotes one value per row, and `WHERE p` may only return rows for which `p` held for that row. Today the leaf-expression extraction passes break both:
```sql
SELECT s, s['a'] AS field
FROM (SELECT named_struct('a', random()) AS s FROM generate_series(1, 3));
```
| `s['a']` | `field` |
|---|---|
| 0.5159112071865757 | 0.4514238291986653 |
| 0.0029104680074608646 | 0.28979983332288195 |
| 0.48542729227457915 | 0.04499392663566881 |
`field` is defined as `s['a']` but differs from it on every row, because the plan is
```
Projection: named_struct(Utf8("a"), random()) AS s, random() AS field
```
The `Filter` form is worse — it returns rows that fail their own predicate:
```sql
SELECT bool_and(s['a'] > 0.5)
FROM (SELECT s FROM (SELECT named_struct('a', random()) AS s FROM generate_series(1, 1000))
WHERE s['a'] > 0.5);
-- false; the predicate tested a different draw than the one in the returned `s`
```
Setting `datafusion.optimizer.enable_leaf_expression_pushdown = false` returns the correct answer in both cases, so the rewrite alone changes the meaning of the query.
**Root cause.** `build_extraction_projection_impl` merges an extraction into the input projection by resolving column references through `build_projection_replace_map`, i.e. by inlining each referenced column's *defining* expression. Inlining a volatile definition produces a second, independent evaluation. There was no volatility check in the file.
This is the same invariant `FileScanConfig::try_swapping_with_projection` already enforces for the physical projection-pushdown path via `would_duplicate_costly_exprs` (#23220) — the logical extraction path was missing it.
## What changes are included in this PR?
- `volatile_output_columns()` — a projection's output columns whose definition is volatile.
- `would_duplicate_volatile()` — true when an extraction references one of them.
- The guard is applied at the three places that can merge into an input projection: `extract_from_plan` (pass 1: Filter/Sort/Limit/Aggregate/Join), `split_and_push_projection` (pass 2), and `try_push_into_inputs` (multi-input/Union routing). Each already had a "leave the plan alone" return path.
The guard is targeted rather than blanket: for `ORDER BY s['a']` the extraction still happens, stacked above the volatile projection instead of merged into it, so the optimization is kept and the result is correct.
### Relationship to #23691
@fornwall wondered on the issue whether #23691 already covers this. I checked out that branch and ran both shapes against it: `SELECT s, s['a']` is incidentally fixed there, but `WHERE s['a'] > 0.5` still duplicates `random()` and still returns rows failing the predicate. #23691 guards `KeepInPlace` *compute cost* in `split_and_push_projection` only; volatility is a separate concern (one duplication is already wrong, regardless of cost or placement) and pass 1 is a different code path. The two changes look independent to me and I believe they compose, but I'd appreciate a second opinion on that from whoever reviews #23691.
## Are these changes tested?
Yes.
- `datafusion/sqllogictest/test_files/projection_pushdown.slt` — a new section beside the existing #23220 volatile section: two `EXPLAIN`s pinning `random()` to a single occurrence, and two deterministic `bool_and(...)` correctness queries. All four fail on `main` and pass here.
- Two rule-level snapshot tests in `extract_leaf_expressions.rs` covering the pass-2 projection merge and the pass-1 `Filter` extraction, using a new test-only `PlacementTestUDF::with_volatility()`.
`cargo test -p datafusion-optimizer` (796), `cargo test -p datafusion --lib --tests` (2049) and the full 504-file sqllogictest suite all pass; `cargo fmt --all` and `cargo clippy --all-targets --all-features -- -D warnings` are clean.
## Are there any user-facing changes?
Queries that were silently returning wrong results now return correct ones. No API change. In the affected shapes the extraction is skipped, which can cost a small amount of column pruning — only when the referenced column is defined by a volatile expression.
---
<sub>Per the ASF generative-tooling policy and DataFusion's AI-assisted contribution guidance: this patch was prepared with AI assistance. The core idea is the one described above — the merge path inlines a referenced column's defining expression, which duplicates a volatile definition — and the open question about how this composes with #23691 is flagged deliberately rather than glossed over.</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on code in PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#discussion_r3905927400
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -1215,6 +1288,15 @@ fn try_push_into_inputs(
if per_input[idx].pairs.is_empty() {
new_inputs.push(input.clone());
} else {
+ // Merging into an input projection inlines the referenced columns'
+ // definitions; bail out when that would duplicate a volatile
+ // computation.
+ if would_duplicate_volatile(
Review Comment:
Added in e07285a. The new sqllogictest covers the projection-above-join route where the left input materializes `named_struct(a, random()) AS s` and the outer projection extracts `l.s[a]`; it asserts `bool_and(field = s[a])`, so the extracted value must come from that join side’s materialized struct rather than a duplicate volatile evaluation.
Validation:
- `cargo test -p datafusion-optimizer extract_leaf_expressions --lib`
- `cargo test --test sqllogictests -- projection_pushdown`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#issuecomment-5494175662
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-optimizer v55.0.0 (current)
Built [ 22.024s] (current)
Parsing datafusion-optimizer v55.0.0 (current)
Parsed [ 0.025s] (current)
Building datafusion-optimizer v55.0.0 (baseline)
Built [ 21.395s] (baseline)
Parsing datafusion-optimizer v55.0.0 (baseline)
Parsed [ 0.027s] (baseline)
Checking datafusion-optimizer v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.193s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 45.079s] datafusion-optimizer
Building datafusion-sqllogictest v55.0.0 (current)
Built [ 78.675s] (current)
Parsing datafusion-sqllogictest v55.0.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-sqllogictest v55.0.0 (baseline)
Built [ 78.738s] (baseline)
Parsing datafusion-sqllogictest v55.0.0 (baseline)
Parsed [ 0.020s] (baseline)
Checking datafusion-sqllogictest v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.098s] 223 checks: 220 pass, 3 fail, 0 warn, 31 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.50.0/src/lints/function_missing.ron
Failed in:
function datafusion_sqllogictest::run_each_configuration, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4be69d4c4ee49bfd27a7635bb414ddc2ecec58ef/datafusion/sqllogictest/src/config_matrix.rs:101
function datafusion_sqllogictest::test_configurations, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4be69d4c4ee49bfd27a7635bb414ddc2ecec58ef/datafusion/sqllogictest/src/config_matrix.rs:81
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It 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.50.0/src/lints/inherent_method_missing.ron
Failed in:
TestContext::apply_config_overrides, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4be69d4c4ee49bfd27a7635bb414ddc2ecec58ef/datafusion/sqllogictest/src/test_context.rs:256
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct 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.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_sqllogictest::TestConfiguration, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/4be69d4c4ee49bfd27a7635bb414ddc2ecec58ef/datafusion/sqllogictest/src/config_matrix.rs:45
Summary semver requires new major version: 3 major and 0 minor checks failed
Finished [ 160.401s] datafusion-sqllogictest
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#issuecomment-5494377546
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24720?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.07921%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.45%. Comparing base ([`a38bb10`](https://app.codecov.io/gh/apache/datafusion/commit/a38bb10c33cff4e99a2707059df99f6971c96f33?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`52d2a85`](https://app.codecov.io/gh/apache/datafusion/commit/52d2a856ba8a8c6d0ec8155cb3b68ccd5bba4e63?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 52 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24720?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tafusion/optimizer/src/extract\_leaf\_expressions.rs](https://app.codecov.io/gh/apache/datafusion/pull/24720?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fextract_leaf_expressions.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL2V4dHJhY3RfbGVhZl9leHByZXNzaW9ucy5ycw==) | 91.66% | [0 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24720?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24720 +/- ##
=======================================
Coverage 81.45% 81.45%
=======================================
Files 1120 1120
Lines 401289 401388 +99
Branches 401289 401388 +99
=======================================
+ Hits 326871 326964 +93
+ Misses 55295 55292 -3
- Partials 19123 19132 +9
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24720?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "jaideeppyne (via GitHub)" <gi...@apache.org>.
jaideeppyne commented on PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#issuecomment-5496733524
I added the non-blocking join-side regression in e07285a and resolved the thread. The new sqllogictest asserts the extracted field still equals the same join side’s materialized `s[a]` when that side builds `named_struct(a, random()) AS s`. Both the optimizer unit target and focused `projection_pushdown` sqllogictest passed locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#issuecomment-5520062788
🚀
@jaideeppyne
Thank you for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24720:
URL: https://github.com/apache/datafusion/pull/24720
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: do not duplicate volatile expressions when extracting leaf expressions [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24720:
URL: https://github.com/apache/datafusion/pull/24720#discussion_r3903780289
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -1215,6 +1288,15 @@ fn try_push_into_inputs(
if per_input[idx].pairs.is_empty() {
new_inputs.push(input.clone());
} else {
+ // Merging into an input projection inlines the referenced columns'
+ // definitions; bail out when that would duplicate a volatile
+ // computation.
+ if would_duplicate_volatile(
Review Comment:
Could we add a join-side regression test for this routed-input guard? The current tests cover the pass-1 and direct-projection paths, but not the case where a projection above a join routes `s['a']` into one join input that is itself `SELECT named_struct('a', random()) AS s`. An execution assertion checking that the extracted value equals that side's `s['a']` would give us coverage for this distinct call site. This is non-blocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org