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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
