Skip to content

Commit 86ba5e0

Browse files
authored
docs(physical-optimizer): warn that wrapper rules must forward schema_check (#25357)
## Which issue does this PR close? Closes #25316. ## Rationale for this change `OptimizationInvariantChecker` consults `schema_check()` on the rule the optimizer holds. For a rule that runs *other* rules inside its own `optimize()` — an instrumentation wrapper, a conditional wrapper, a rule that fuses a short sequence — the optimizer never sees the inner rules, only the wrapper. Returning `false` from such a wrapper therefore disables schema validation for everything it wraps, including rules that explicitly opted in. Nothing warns at registration or at run time; validation simply stops happening, and the loss only surfaces if a schema-changing regression happens to hit that path. I walked into this myself: `false` looked correct because each wrapped rule had its own `schema_check()`, but those values are never consulted once the rules are inside a wrapper. ## What changes are included in this PR? A doc comment on `schema_check()` stating the requirement, with the one-line `any()` form, so an implementor reads it where they make the decision. ## Are these changes tested? Documentation only, no behavior change. ## Are there any user-facing changes? Doc only.
1 parent 5a44905 commit 86ba5e0

1 file changed

Lines changed: 29 additions & 0 deletions

File tree

‎datafusion/session/src/physical_optimizer.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,5 +80,34 @@ pub trait PhysicalOptimizerRule: Debug + std::any::Any {
8080
/// change the schema of the plan after the rewriting.
8181
/// Some of the optimization rules might change the nullable properties of the schema
8282
/// and should disable the schema check.
83+
///
84+
/// The planner reads this from the rule it holds, so a rule that runs
85+
/// *other* rules inside its own [`optimize`] must account for what the
86+
/// whole wrapped transformation does to the schema, not just its own body:
87+
///
88+
/// * A wrapper around a single rule should forward that rule's value.
89+
/// * A wrapper that runs multiple rules should return `true` only if the
90+
/// complete wrapped transformation preserves the schema contract. When
91+
/// that is derived solely from the wrapped rules, every wrapped rule must
92+
/// enable the check (`all()`, not `any()`): if any wrapped rule is allowed
93+
/// to change the schema, returning `true` makes the checker assert a
94+
/// change it should tolerate.
95+
/// * A wrapper mixing schema-preserving and schema-changing rules cannot be
96+
/// expressed as a single flag; validate per-rule inside the wrapper so the
97+
/// inner checks are retained.
98+
///
99+
/// When the flag can be derived from the wrapped rules, `all()` (not
100+
/// `any()`) is the safe combinator:
101+
///
102+
/// ```text
103+
/// fn schema_check(&self) -> bool {
104+
/// self.wrapped.iter().all(|rule| rule.schema_check())
105+
/// }
106+
/// ```
107+
///
108+
/// Returning a blanket `false` still silently disables validation for
109+
/// everything inside, including rules that asked for it.
110+
///
111+
/// [`optimize`]: PhysicalOptimizerRule::optimize
83112
fn schema_check(&self) -> bool;
84113
}

0 commit comments

Comments
 (0)