Repository navigation
Check the final physical plan in the planner and name the rule that broke it #26461
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,10 @@ use datafusion_physical_expr::expressions::Literal; | |
| use datafusion_physical_expr::{ | ||
| LexOrdering, PhysicalSortExpr, create_physical_sort_exprs, | ||
| }; | ||
| use datafusion_physical_optimizer::ensure_requirements::EnsureRequirements; | ||
| use datafusion_physical_optimizer::sanity_checker::{ | ||
| check_plan_tree_sanity, input_requirements_hold, | ||
| }; | ||
| use datafusion_physical_plan::empty::EmptyExec; | ||
| use datafusion_physical_plan::execution_plan::InvariantLevel; | ||
| use datafusion_physical_plan::joins::PiecewiseMergeJoinExec; | ||
|
|
@@ -3391,6 +3395,10 @@ impl DefaultPhysicalPlanner { | |
|
|
||
| let mut new_plan = Arc::clone(&plan); | ||
| let optimizer_context = SessionOptimizerContext::new(session_state); | ||
| // The plan after each rule, so that a failed final check can name the | ||
| // rule that broke the plan. Plans share unchanged subtrees, so this | ||
| // costs one pointer per rule. | ||
| let mut plan_after_rule = Vec::with_capacity(optimizers.len()); | ||
| for optimizer in optimizers { | ||
| let before_schema = new_plan.schema(); | ||
| new_plan = optimizer | ||
|
|
@@ -3408,12 +3416,16 @@ impl DefaultPhysicalPlanner { | |
| optimizer.name(), | ||
| displayable(new_plan.as_ref()).indent(false) | ||
| ); | ||
| observer(new_plan.as_ref(), optimizer.as_ref()); | ||
| plan_after_rule.push((optimizer, Arc::clone(&new_plan))); | ||
| } | ||
|
|
||
| // This runs once after all optimizer runs are complete, including | ||
| // rules appended by the user, to verify that the plan is executable. | ||
| let optimizer_options = &session_state.config_options().optimizer; | ||
| if let Err(e) = check_plan_tree_sanity(&new_plan, optimizer_options) { | ||
|
Contributor
There was a problem hiding this comment. Since this check can no longer be skipped, an opt-out flag could be a follow-up if a downstream needs it. |
||
| return Err(sanity_check_error(e, &new_plan, &plan, &plan_after_rule)); | ||
| } | ||
|
|
||
| debug!( | ||
| "Optimized physical plan:\n{}\n", | ||
|
|
@@ -3660,6 +3672,69 @@ fn tuple_err<T, R>(value: (Result<T>, Result<R>)) -> Result<(T, R)> { | |
| } | ||
| } | ||
|
|
||
| /// A physical optimizer rule and the plan it produced. | ||
| type PlanAfterRule<'a> = ( | ||
| &'a Arc<dyn PhysicalOptimizerRule + Send + Sync>, | ||
| Arc<dyn ExecutionPlan>, | ||
| ); | ||
|
|
||
| /// Adds context to an error from the final [`check_plan_tree_sanity`]. | ||
| /// | ||
| /// `EnsureRequirements` makes the order and distribution requirements hold, | ||
| /// and every rule after it must keep them. When they do not hold for | ||
| /// `final_plan`, the context names the rule that broke this contract: either | ||
| /// `EnsureRequirements` itself, or the rule after which the requirements | ||
| /// stopped holding. | ||
| fn sanity_check_error( | ||
| error: DataFusionError, | ||
| final_plan: &Arc<dyn ExecutionPlan>, | ||
| input_plan: &Arc<dyn ExecutionPlan>, | ||
| plan_after_rule: &[PlanAfterRule], | ||
| ) -> DataFusionError { | ||
| const SANITY_CHECK: &str = "SanityCheckPlan"; | ||
| if input_requirements_hold(final_plan) { | ||
| // The error is about unbounded inputs or another invariant. | ||
| return DataFusionError::Context(SANITY_CHECK.to_string(), Box::new(error)); | ||
| } | ||
|
|
||
| let ensure_requirements = EnsureRequirements::new(); | ||
| let enforced = plan_after_rule | ||
| .iter() | ||
| .rposition(|(rule, _)| rule.name() == ensure_requirements.name()); | ||
|
Contributor
There was a problem hiding this comment. Once #26757 lands, this search could start at the enforcement phase boundary instead of matching
Contributor
Author
There was a problem hiding this comment. #26757 is still a draft, so I'll make that switch there: once it is rebased on this PR, the search will start at the analyzer/optimizer boundary and the name match goes away. |
||
| // Plans before `EnsureRequirements` need not satisfy the requirements, so | ||
| // only plans from its last run onwards are searched. Without it, every | ||
| // plan is searched. | ||
| let (searched, before_searched) = match enforced { | ||
| Some(idx) => (&plan_after_rule[idx..], None), | ||
| None => (plan_after_rule, Some(input_plan)), | ||
| }; | ||
| let broken_by = match searched | ||
| .iter() | ||
| .rposition(|(_, plan)| input_requirements_hold(plan)) | ||
| { | ||
| Some(idx) => searched.get(idx + 1), | ||
| None if before_searched.is_some_and(input_requirements_hold) => searched.first(), | ||
| None => None, | ||
| }; | ||
| let context = match (broken_by, enforced) { | ||
| (Some((rule, _)), _) => format!( | ||
| "{SANITY_CHECK}: order and distribution requirements stopped holding \ | ||
| after physical optimizer rule '{}'", | ||
| rule.name() | ||
| ), | ||
| (None, Some(_)) => format!( | ||
| "{SANITY_CHECK}: the plan produced by physical optimizer rule '{}' \ | ||
| does not satisfy the order and distribution requirements", | ||
| ensure_requirements.name() | ||
| ), | ||
| (None, None) => format!( | ||
| "{SANITY_CHECK}: no physical optimizer rule produced a plan that \ | ||
| satisfies the order and distribution requirements" | ||
| ), | ||
| }; | ||
| DataFusionError::Context(context, Box::new(error)) | ||
| } | ||
|
|
||
| struct OptimizationInvariantChecker<'a> { | ||
| rule: &'a Arc<dyn PhysicalOptimizerRule + Send + Sync>, | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this document is outdated and redundand and hard to find -- maybe as a follow on PR we can fold it into the documentation in #26351
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about this document 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. #26351 has merged, so a follow-up PR can move this list next to the contract docs.
This PR is now rebased on it. I also removed
SanityCheckPlanfrom the contract's list of rules that must keep a fixed order, and added a sentence saying the planner checks the plan after the last rule.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 😆