ozankabak commented on code in PR #13986:
URL: https://github.com/apache/datafusion/pull/13986#discussion_r1901204911
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -110,6 +110,16 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
/// trait, which is implemented for all `ExecutionPlan`s.
fn properties(&self) -> &PlanProperties;
+ /// Returns an error if this individual node does not conform to its
invariants.
+ /// These invariants are typically only checked in debug mode.
+ ///
+ /// A default set of invariants is provided in the default implementation.
+ /// Extension nodes can provide their own invariants.
+ fn check_node_invariants(&self) -> Result<()> {
+ // TODO
Review Comment:
Conceptually, sanity checking is a "more general" process -- it verifies
that any two operators that exchange data (i.e. one's output feeds the other's
input) are compatible. So I don't think we can "change" it to be an invariant
checker, but we can extend it to also check "invariants" of each individual
operator (however they are defined by an `ExecutionPlan`) as it traverses the
plan tree.
However, we can not blindly run sanity checking after every rule. Why?
Because rules have the following types regarding their input/output plan
validity:
- Some rules only take in valid plans and output valid plans (e.g.
`ProjectionPushdown`). These are typically applied at later stages in the
optimization/plan construction process.
- Some take in invalid or valid plans, and always create valid plans (e.g.
`EnforceSorting` and `EnforceDistribution`). These can be applied any time, but
are typically applied in the middle of the optimization/plan construction
process.
- Some take invalid plans and yield still invalid plans (IIRC
`JoinSelection` is this way). These are typically applied early in the
optimization/plan construction process.
As of this writing, we don't have a formal cut-off point in our list of
rules whereafter plans remain valid, but I suspect they do after
`EnforceSorting`. In debug/upgrade mode, we can apply `SanityCheckPlan` after
every rule after that point.
--
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]
