zhuqi-lucas opened a new issue, #25316:
URL: https://github.com/apache/datafusion/issues/25316
### Is your feature request related to a problem or challenge?
`PhysicalOptimizerRule::schema_check()` is consulted by
`OptimizationInvariantChecker` on the rule object the optimizer holds. That
works well for leaf rules, but it silently loses validation for *wrapper* rules
— a rule that holds one or more other rules and runs them inside its own
`optimize()`.
Wrapper rules are a natural pattern and already exist in the wild:
- instrumentation wrappers (timing, tracing, per-rule metrics),
- conditional wrappers that run an expensive pass only when a cheaper one
changed the plan,
- fusion wrappers that group a small sequence of rules to avoid redundant
traversals.
When such a wrapper is registered, the optimizer no longer sees the inner
rules at all: it calls `schema_check()` on the wrapper. If the wrapper returns
`false` — an easy and seemingly harmless choice, e.g. "the inner rules validate
themselves" — then the schema validation of *every rule it wraps* disappears,
including rules that explicitly opted into it.
Two properties make this a sharp edge:
1. **It fails silently.** Nothing warns at registration or at run time;
validation simply stops happening. A plan that violates the schema contract is
only caught later, if at all.
2. **It is invisible in tests.** Schema-changing regressions need the exact
plan shape to surface, so a wrapper can drop validation for a long time without
any test turning red.
I hit exactly this while writing a conditional wrapper: returning `false`
looked correct because each inner rule had its own `schema_check()`, but those
inner values are never consulted once the rules are wrapped. The fix on my side
was to report `true` when any wrapped rule does, which is easy *once you know*.
The trait gives no hint that this is required.
### Describe the solution you'd like
Cheapest first; the first item alone would have prevented the bug:
1. **Document the contract on `schema_check()`**: a rule that runs other
rules inside `optimize()` must forward their requirement (in practice, `any()`
over the wrapped rules), because the optimizer only consults the outermost
rule. One sentence in the doc comment.
2. **Debug-time detection.** In `OptimizationInvariantChecker`, when
`schema_check()` is `false`, still compare the schemas under `debug_assertions`
and emit a warning (not an error) if they differ. That surfaces both a wrapper
that dropped validation and a leaf rule that opted out but changes schemas
anyway, without costing anything in release builds.
3. **Optional, larger: let the trait express composition.** Something like
```rust
/// Rules this rule runs internally, if any.
fn inner_rules(&self) -> &[Arc<dyn PhysicalOptimizerRule + Send + Sync>]
{ &[] }
```
so the checker can derive the effective requirement itself rather than
relying on every wrapper author to remember. This is a bigger surface change
and probably deserves its own discussion — raising it here mainly to ask
whether it is wanted before anyone builds it.
### Describe alternatives you've considered
Leaving it as is and relying on review to catch wrapper authors: that is
what happened in my case (a reviewer spotted it), but it depends on the
reviewer knowing this specific interaction.
Making `schema_check()` default to `true` would be safer but changes
behavior for existing rules that intentionally opt out.
### Additional context
Happy to send a PR for 1 and 2 if that sounds reasonable.
--
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]
