Conversation
Five guards in the logical optimizer stop a rule from evaluating a column definition more than one time. Each one was added after a bug report, and each one covers a single call site. This adds the same property as a plan invariant, next to the schema invariant in `assert_valid_optimization`. After a rule runs, the number of evaluation sites of a volatile call, and of a `KeepInPlace` call, must not be higher than before the rule. Counts are per row lineage, so a predicate that is duplicated across the branches of a union, or across the two sides of a join, is one evaluation. Both switches are off, so there is no behaviour change and no cost. Flip `CHECK_VOLATILE_EVALUATIONS` or `CHECK_KEPT_EVALUATIONS` to reproduce the findings listed in the pull request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 18, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25458 +/- ##
==========================================
+ Coverage 82.33% 82.35% +0.01%
==========================================
Files 1137 1138 +1
Lines 432498 433110 +612
Branches 432498 433110 +612
==========================================
+ Hits 356115 356675 +560
- Misses 54844 54854 +10
- Partials 21539 21581 +42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…nd of the run Optimizer rules cooperate. `simplify_expressions` expands `x BETWEEN a AND b` into two references to `x`, and `CommonSubexprEliminate` hoists the duplicate back out in the same pass. A check after every rule reports that pair, and the query never pays for it. So the check now runs one time, at the end of `Optimizer::optimize`, against the plan the optimizer was given. The per rule hook is gone. Only the volatile half is an invariant. Two evaluations of a volatile call give two different answers, so a new site is a wrong results bug. A second evaluation site of a `KeepInPlace` call is a cost, and pushing a predicate through a projection that computes one is a decided trade-off, so it is no longer counted. That removes the second switch and the `SiteKind` axis. Sites are keyed by the call with every column reduced to its unqualified name, not by its printed text. A rule that duplicates a call and re-qualifies its columns, from `f(a)` to `f(test.a)`, no longer escapes the check. One `const` switch, off, because `random() BETWEEN 0.0 AND 1.0` in `expr.slt` still fails, which is apache#25457. The check itself is tested while the switch is off, by a unit test that builds a before and after plan pair and asserts the error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jayzhan211
reviewed
Sep 19, 2026
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
missing test
enabled() is const false → the two call sites in Optimizer::optimize never run in CI, and nothing tests the check against a real optimizer run. Pin the known failure so the test flips when #25457 is fixed (I ran this shape; it reports 2 sites against 1 today):
/// https://github.com/apache/datafusion/issues/25457. Flip to `?` once fixed.
#[test]
fn reports_between_expansion_of_a_kept_volatile_call() -> Result<()> {
let udf = ScalarUDF::new_from_impl(
PlacementTestUDF::new()
.with_placement(ExpressionPlacement::KeepInPlace)
.with_volatility(Volatility::Volatile),
);
let input = LogicalPlanBuilder::from(test_table_scan()?)
.filter(udf.call(vec![col("a")]).between(lit(1u32), lit(5u32)))?
.build()?;
let before = EvaluationSites::count(&input);
let optimized =
Optimizer::new().optimize(input, &OptimizerContext::new(), |_, _| {})?;
let error = before.check(&optimized).unwrap_err().to_string();
assert!(error.contains("is evaluated at 2 sites, against 1"), "{error}");
Ok(())
}
Comment on lines
+269
to
+281
| expr.clone() | ||
| .transform(|expr| { | ||
| Ok(match expr { | ||
| Expr::Column(column) => { | ||
| Transformed::yes(Expr::Column(Column::new_unqualified(column.name))) | ||
| } | ||
| other => Transformed::no(other), | ||
| }) | ||
| }) | ||
| .map(|transformed| transformed.data.to_string()) | ||
| // The rewrite above never fails. Fall back to the printed text rather | ||
| // than fail an invariant check for it. | ||
| .unwrap_or_else(|_| expr.to_string()) |
Contributor
There was a problem hiding this comment.
Suggested change
| expr.clone() | |
| .transform(|expr| { | |
| Ok(match expr { | |
| Expr::Column(column) => { | |
| Transformed::yes(Expr::Column(Column::new_unqualified(column.name))) | |
| } | |
| other => Transformed::no(other), | |
| }) | |
| }) | |
| .map(|transformed| transformed.data.to_string()) | |
| // The rewrite above never fails. Fall back to the printed text rather | |
| // than fail an invariant check for it. | |
| .unwrap_or_else(|_| expr.to_string()) | |
| unnormalize_col(expr.clone()).to_string() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
StructArray#23655. It does not fix them. It adds the check that finds them, and it found BETWEEN evaluates its operand two times, so a volatile operand returns wrong rows #25457.Rationale for this change
A query must not evaluate a volatile function more times than the query text says.
random()gives a different answer each time, so a second evaluation returns wrong rows.Five guards in the logical optimizer exist only to keep that property. Each one was added after a bug report, and each one covers one call site:
would_duplicate_volatileinextract_leaf_expressions, 3 call sitesProjectionbranch ofpush_down_filterAggregatebranch ofpush_down_filtermerge_would_duplicate_kept_exprinextract_leaf_expressionsoptimize_projectionsThe next shape is always open. #25329 is the shape none of the five covers. #25456 replaces the five guards with one policy, which is the fix. This PR adds the check that tells you when the policy has a hole.
What changes are included in this PR?
A new crate private module,
datafusion/optimizer/src/evaluation_sites.rs, and one call at the end ofOptimizer::optimize.The check runs one time, over the whole run. It compares the final plan against the plan the optimizer was given. Intermediate plans are not checked, because rules cooperate.
simplify_expressionsexpandsx BETWEEN a AND binto two references tox, andCommonSubexprEliminatehoists the duplicate back out in the same pass. A check after every rule reports that pair, and the query never pays for it. Only the plan that the user pays for matters, so only that plan is checked. The price is that the message does not name the rule at fault. The expression and the before and after site counts are enough to start from.Volatile calls only. Two evaluations of a volatile call give two different answers, so a new site is always a wrong results bug. That is an invariant. A second evaluation site of a
KeepInPlacecall is a cost, not a wrong answer, and pushing a predicate through a projection that computes one is a decided trade-off, because the predicate can then reach the scan. See the decisions table of the EPIC. So aKeepInPlacesite is not counted, and the second switch of the earlier version is gone.Sites are keyed structurally. The key is the text of the call with every column reduced to its unqualified name. A rule may re-qualify the columns of a call while it moves it, so
f(a)andf(test.a)must be the same call. The earlier version keyed by printed text, and a rule that duplicates a call and re-qualifies it escaped the check.Counts are per row lineage.
sites(node) = own_sites(node) + max over inputs of sites(input). The maximum, not the sum, because a row that enters the left input of a join never meets the nodes of the right input. This is what makes union branches, join sides and a filter pushed below a join free of reports. A subquery is another lineage. A call that is not in the input plan is not checked, because a rule is free to rewriteconcat_wsintoconcat, or one Spark function into two DataFusion functions. That is a new call, not a second evaluation of an old one.The error message names the expression and the two counts:
Measured results
The check ran over the full sqllogictest suite, 520 files, with
ENFORCE_NO_NEW_VOLATILE_EVALUATIONSset totrue. It reports one query:expr.slt:1005SELECT random() BETWEEN 0.0 AND 1.0, random() = random()random()3 -> 4That is a wrong results bug, filed as #25457.
SELECT count(*) FROM v WHERE random() BETWEEN 0.4 AND 0.6over 100000 rows returns 35751 rows, against the 20000 that one draw gives.The per rule version of this check reported 14 queries in 7 files. The other 13 are gone, and each one shows why the end of run check is the right default:
abs(c1) BETWEEN ...inselect.sltandCOALESCE(CAST(array_has(...)))inarray/array_has.slt:simplify_expressionsduplicates the operand, andCommonSubexprEliminatehoists it back in the same pass. The final plan evaluates it one time.file_row_index()infile_row_index.slt:CommonSubexprEliminatehoists the call, andoptimize_projectionsputs it back. The final plan has the site count the input plan has. The classification order that lets a volatile call be inlined at all is fixed by fix: one inlining policy for the rules that inline a projection column #25456.KeepInPlaceshapes: not counted any more, because they are not an invariant.The check costs two plan walks per optimizer run. That is not measurable on the sqllogictest suite. Wall clock of
cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests, best of three on 12 threads: 9.92 s with the switch off, 9.23 s with the switch on. The difference is inside the run to run noise of this machine. The per rule version cost 12% of the CPU of the same suite.Correction: an earlier version of this description claimed that
[profile.ci.package."*"]turns debug assertions off insidedatafusion-optimizer. That is wrong. The override applies to non-member dependencies only, anddebug_assertionsis on for workspace members under--profile ci. The existingcheck_invariantscall does run in the sqllogictest suite. This PR enforces the invariant in debug builds only, which is where the other plan invariants are checked too.What is the testing strategy for this PR?
Nine unit tests in the new module. Eight cover the counting rules directly: one node, a chain, a
KeepInPlacecall that is not counted, union branches, join sides, a subquery, the key over a qualified column, and a call that the optimizer invented. The ninth calls the check itself on a hand built before and after plan pair and asserts that it fails with the expected message, so the machinery has a test while the switch is off.cargo test --profile ci -p datafusion-optimizerpasses 891 + 26 + 5 tests. The full sqllogictest suite is green over all 520 files with the switch off, as this PR ships it.Are there any user-facing changes?
No. The switch is off, so there is no behaviour change and no cost.
EvaluationSites::ofreturns an empty map and walks nothing. There is no new public API.Part of the leaf-pushdown EPIC: #25459
🤖 Generated with Claude Code