alamb commented on code in PR #26757:
URL: https://github.com/apache/datafusion/pull/26757#discussion_r4468820478
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -157,6 +159,9 @@ struct SessionStateInner {
type_planner: Option<Arc<dyn TypePlanner>>,
/// Responsible for optimizing a logical plan
optimizer: Optimizer,
+ /// Rules that run before requirements are enforced, ending with
+ /// `EnsureRequirements`
Review Comment:
nit: I don't think we need to mention `EnsureRequirements` here as it is an
implementation detail
##########
datafusion/core/src/optimizer_rule_reference.md:
##########
@@ -64,36 +91,49 @@ Rule order matters. The default pipeline may change between
releases.
| 25 | `push_down_leaf_projections` | Pushes the helper
projections created by leaf extraction toward leaf inputs.
|
| 26 | `optimize_projections` | Prunes unused columns
and removes unnecessary logical projections.
|
+### Physical Analyzer Rules
Review Comment:
I personally think this information should be in the doc comments and not a
separate markdown file for consistency
or example, this information probably would be better if it were directly on
the PhysicalOptimizer doc comments. It would be more likely discoverable.
I have some draft AI generated PR to do it here but I haven't cleaned it up.
- https://github.com/apache/datafusion/pull/26899
##########
datafusion/core/src/optimizer_rule_reference.md:
##########
@@ -64,36 +91,49 @@ Rule order matters. The default pipeline may change between
releases.
| 25 | `push_down_leaf_projections` | Pushes the helper
projections created by leaf extraction toward leaf inputs.
|
| 26 | `optimize_projections` | Prunes unused columns
and removes unnecessary logical projections.
|
+### Physical Analyzer Rules
+
+Physical analyzer rules run before requirements are enforced. They run as
+their own phase before the physical optimizer rules and end with
+`EnsureRequirements`, so every physical optimizer rule receives a valid plan. A
+rule runs in this phase when `EnsureRequirements` reads what it changes, such
+as the join mode `join_selection` picks, or when it needs the plan before
+enforcement adds repartitions and sorts, such as `WindowTopN`. Both phases hold
+performance rules. A rule is either a `PhysicalAnalyzerRule` or a
+`PhysicalOptimizerRule`, so each rule appears in one list only.
+
+| order | rule | summary
|
+| ----- | ---------------------------- |
-------------------------------------------------------------------------------------------------------
|
+| 1 | `AddOutputRequirements` | Adds helper nodes so output
requirements survive enforcement and later physical rewrites. |
Review Comment:
I think this doc would already alread basically auto generated by rust docs
For example in the logical analyzer rules
https://docs.rs/datafusion/latest/datafusion/optimizer/trait.AnalyzerRule.html
https://docs.rs/datafusion/latest/datafusion/optimizer/index.html
<img width="1373" height="1312" alt="Image"
src="https://github.com/user-attachments/assets/5c6f6d01-1880-4c3a-9e34-f25db82d0594";
/>
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3428,27 +3430,36 @@ impl DefaultPhysicalPlanner {
))
}
- /// Optimize a physical plan by applying each physical optimizer,
- /// calling observer(plan, optimizer after each one)
+ /// Run the two physical planning phases over `plan`, calling
+ /// `observer(plan, rule_name)` after each rule.
+ ///
+ /// The analyzer phase runs first and in full; it ends with requirement
Review Comment:
I think it would be clearer if the high level flow of the optimizer was
explained in the PhsyicalOptimizer struct docs, and then this method could just
refer to it. Otherwise we are spreading redundant information all over the
codebase
##########
datafusion/core/src/optimizer_rule_reference.rs:
##########
@@ -60,6 +61,17 @@ fn logical_rules_match_documented_order() {
assert_eq!(documented_rules("### Logical Optimizer Rules"), rules);
}
+#[test]
Review Comment:
I know this just follows the same pattern, so we should just leave it for
this PR, but having an entirely separate doc / build / test system for
optimizer rules seems confusing at best to me
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3428,27 +3430,36 @@ impl DefaultPhysicalPlanner {
))
}
- /// Optimize a physical plan by applying each physical optimizer,
- /// calling observer(plan, optimizer after each one)
+ /// Run the two physical planning phases over `plan`, calling
+ /// `observer(plan, rule_name)` after each rule.
+ ///
+ /// The analyzer phase runs first and in full; it ends with requirement
+ /// enforcement, so every [`PhysicalOptimizerRule`] in the optimizer phase
+ /// receives a valid plan. See [`PhysicalAnalyzerRule`] for the phase
+ /// diagram.
///
- /// Before the optimizers run, each [`MaterializedCteScanExec`] is bound
+ /// Before the rules run, each [`MaterializedCteScanExec`] is bound
Review Comment:
the details about MaterializedCte seems irrelevant (though not added by this
PR)
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3729,41 +3772,39 @@ fn sanity_check_error(
return plain_error(error);
}
- let ensure_requirements = EnsureRequirements::new();
- let enforced = optimizers
- .iter()
- .rposition(|rule| rule.name() == ensure_requirements.name());
- // Plans before `EnsureRequirements` need not satisfy the requirements, so
- // only plans from its last run onwards are checked. Without it, every
- // plan is checked, starting with the input plan.
- let first_checked = enforced.unwrap_or(0);
- let mut held = enforced.is_none() && input_requirements_hold(&input_plan);
- let mut broken_by = None;
+ // Plans inside the analyzer phase need not satisfy the requirements, so
+ // only its output and the plans after it are checked.
let mut plan = input_plan;
- for (idx, rule) in optimizers.iter().enumerate() {
- plan = match rule.optimize_with_context(plan, optimizer_context) {
+ for rule in analyzers {
Review Comment:
this is much nicer
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -3774,13 +3815,33 @@ fn sanity_check_error(
}
struct OptimizationInvariantChecker<'a> {
- rule: &'a Arc<dyn PhysicalOptimizerRule + Send + Sync>,
+ /// `PhysicalAnalyzer` or `PhysicalOptimizer`, for error messages.
+ phase: &'static str,
+ /// Name of the rule being checked, for error messages.
+ rule_name: &'a str,
+ /// Whether the rule promised to preserve the schema.
+ schema_check: bool,
}
impl<'a> OptimizationInvariantChecker<'a> {
- /// Create an [`OptimizationInvariantChecker`] that performs checking per
tule.
+ /// Create an [`OptimizationInvariantChecker`] for a
[`PhysicalOptimizerRule`].
pub fn new(rule: &'a Arc<dyn PhysicalOptimizerRule + Send + Sync>) -> Self
{
Review Comment:
maybe renaming this to `new_for_optimizer` would be more consistent
--
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]
