adriangb opened a new pull request, #26873: URL: https://github.com/apache/datafusion/pull/26873
## Which issue does this PR close? - Part of https://github.com/apache/datafusion/issues/25459. Do not merge. Each part merges through its own PR. ## Rationale for this change This branch puts six PRs together and turns the "no new volatile evaluations" invariant on. Purpose: CI and review of the combined effect. | Order | PR | Head | |---|---|---| | 1 | https://github.com/apache/datafusion/pull/25476 (volatile `BETWEEN`) | 1c10c5f21 | | 2 | https://github.com/apache/datafusion/pull/25458 (invariant, switch off) | 3e4827446 | | 3 | https://github.com/apache/datafusion/pull/26281 (filter into scan below extraction projection) | f8c53ed5f | | 4 | https://github.com/apache/datafusion/pull/26872 (CSE convergence), brings https://github.com/apache/datafusion/pull/25456 (one inlining policy) | ddb2155b0, then c09f2bb78 (SPDX header) | | 5 | https://github.com/apache/datafusion/pull/26871 (columns by index) | 80269e625 | ## What changes are included in this PR? Merge conflicts and resolutions: - `push_down_filter.rs` (#26281 with #26872): both added functions at the same place. Kept all of them. - `extract_leaf_expressions.rs` (#25456 with #26871): - Merge guard: kept `ProjectionInliner` + `pinned_for_merge` from #25456. Removed `would_duplicate_volatile` and `merge_would_duplicate_kept_expr` (#26871 had rewritten them by index). Kept the index helpers from #26871. - `build_extraction_projection_impl`: kept the `Option` return and pin check (#25456) and the substitution, dedup and pass-through by index (#26871). - `remap_pairs_and_columns`: `ExtractionTarget` shape from #25456, remap by index from #26871, also for `direct_columns`. - Two #26871 unit tests: new `direct_columns` argument. Other commits: - Switch on: `ENFORCE_NO_NEW_VOLATILE_EVALUATIONS = true`. The pinned `BETWEEN` test is flipped (#25476 fixes it). - SPDX license header in `evaluation_sites.rs` (#25458) and `between.rs` (#25476). - Review fixes: - `get_preimage` skips a volatile argument. `floor(random() * 10) = 3` evaluated `random()` twice. Bug on `main`, found by the switch. Not part of the six PRs. - The three pin checks in `extract_leaf_expressions.rs` use the same column spelling. `route_to_inputs` routes `direct_columns` by index. (merge of #25456 and #26871) ### Blocker for the switch: false positive The invariant keys calls by name. `SELECT f(a), f(y) FROM (SELECT a, a AS y FROM t)` with a volatile UDF `f` fails in debug builds: after the projections merge, both calls are `f(a)`, so the check sees 2 sites against 1. It fires for `MoveTowardsLeafNodes` and `KeepInPlace` UDFs. It does not fire with `random()` (no arguments), so there is no slt case; the unit test `reports_renamed_column_merge_as_a_new_site` pins it. The key must be fixed before the switch can go on. ## What is the testing strategy for this PR? On head 2713a630c: - `cargo fmt --all`; `cargo clippy --all-targets --all-features -D warnings` on optimizer, physical-expr, proto, expr, datafusion: clean. - `cargo test -p datafusion-optimizer -p datafusion-physical-expr -p datafusion-proto`: 3216 passed, 0 failed. - `optimizer_convergence`: 3 passed. - sqllogictests (`--profile ci`): 542/542 files, exit 0. - Leaf-pushdown fuzz, 3000 cases: 4 passed. - Not run: the full extended suite. ## Are there any user-facing changes? No. This PR is not for merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
