xudong963 commented on code in PR #25357:
URL: https://github.com/apache/datafusion/pull/25357#discussion_r4022927161
##########
datafusion/session/src/physical_optimizer.rs:
##########
@@ -80,5 +80,19 @@ pub trait PhysicalOptimizerRule: Debug + std::any::Any {
/// change the schema of the plan after the rewriting.
/// Some of the optimization rules might change the nullable properties of
the schema
/// and should disable the schema check.
+ ///
+ /// The planner reads this from the rule it holds, so a rule that runs
+ /// *other* rules inside its own [`optimize`] must forward their
+ /// requirement — in practice `any()` over the rules it wraps. Returning
+ /// `false` from a wrapper silently disables validation for everything
+ /// inside it, including rules that asked for it:
+ ///
+ /// ```text
+ /// fn schema_check(&self) -> bool {
+ /// self.wrapped.iter().any(|rule| rule.schema_check())
Review Comment:
Better to replace the “in practice any()” claim and example. Suggested
guidance:
A wrapper around one rule should forward that rule’s value. A wrapper that
runs multiple rules should return true only if the complete wrapped
transformation guarantees the schema contract. If this is derived solely from
the wrapped rules, all wrapped rules must enable schema checking. Mixed
wrappers require per-rule validation to retain the inner checks.
--
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]
