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:
- 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.
- 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:
-
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.
-
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.
-
Optional, larger: let the trait express composition. Something like
/// 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.
Is your feature request related to a problem or challenge?
PhysicalOptimizerRule::schema_check()is consulted byOptimizationInvariantCheckeron 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 ownoptimize().Wrapper rules are a natural pattern and already exist in the wild:
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 returnsfalse— 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:
I hit exactly this while writing a conditional wrapper: returning
falselooked correct because each inner rule had its ownschema_check(), but those inner values are never consulted once the rules are wrapped. The fix on my side was to reporttruewhen 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:
Document the contract on
schema_check(): a rule that runs other rules insideoptimize()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.Debug-time detection. In
OptimizationInvariantChecker, whenschema_check()isfalse, still compare the schemas underdebug_assertionsand 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.Optional, larger: let the trait express composition. Something like
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 totruewould 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.