alamb commented on code in PR #26351: URL: https://github.com/apache/datafusion/pull/26351#discussion_r4295449469
########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,85 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending or replacing the default +//! list of optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! It's recommended to keep the default rule order and using supported configuration +//! options to disable optimizations. These variations preserve correctness. For +//! example, `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Custom pipelines +//! require more care, as described below. +//! +//! ## Custom Optimizer Pipelines Review Comment: I found this somewhat confusingly worded. I would find it clearer if we started by stating the invariants: 1. All optimizer rules must preserve plan correctness (the output plan can be run and produces the same results as the input plan). We can note there are some built in plans that aren't yet there (like EnforceRequirements) 2. That reordering optimizer rules (other than the exceptions) should result in a plan that produces the same results Then we could then move on to the explicit warning that adding / reordering rules may make some of the built in rules less effective as some built in rules (e.g. CombinePartialFinalAggregate) match certain plan shapes. p.s. I would love to split out EnforceRequrements into its own Analyzer thing precisely so we don't have to have to mention the special case for EnforceRequirements above ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,85 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending or replacing the default +//! list of optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! It's recommended to keep the default rule order and using supported configuration +//! options to disable optimizations. These variations preserve correctness. For +//! example, `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Custom pipelines +//! require more care, as described below. +//! +//! ## Custom Optimizer Pipelines +//! +//! The following extension pipeline can expect built-in optimizer rules to preserve +//! plan validity, but not to provide complete optimization coverage. +//! +//! ```text +//! // Custom rules +//! let rules = vec![ +//! rule3, +//! extension_rule_1, +//! rule1, +//! ]; +//! ``` +//! +//! - **Plan Validity:** Applying `rule1` after `rule3` should preserve query +//! semantics and return a valid plan without internal errors or invalid +//! plan shapes. +//! +//! The pipeline must still satisfy requirements needed for correctness. For +//! example, [`EnsureRequirements`] enforces input distribution and ordering. +//! Some constraints remain implicit or are documented only in individual rules. +//! +//! - **Optimization Coverage:** A non-default rule order may expose rules to +//! plan shapes they do not recognize, causing them to miss optimizations +//! that would apply in the default pipeline. +//! +//! For example, [`LimitedDistinctAggregation`] runs before [`EnsureRequirements`] +//! and [`CombinePartialFinalAggregate`], so it only targets the canonical shape. To keep +//! its implementation simple, it does not guarantee optimization coverage for +//! non-canonical shapes produced by extension pipelines. +//! +//! ```txt +//! -- Canonical (default pipeline): +//! -- Built-in rule recognize and optimize this shape +//! Limit +//! Aggregate(Final) +//! Aggregate(Partial) +//! +//! -- Non-canonical (repartitioning inserted first): +//! -- Built-in rule skip the optimization +//! Limit +//! Aggregate(Final) +//! Repartition +//! Aggregate(Partial) +//! ``` +//! +//! Contributions to DataFusion's built-in rules for custom optimizer pipelines +//! should follow these guidelines: +//! +//! - Downstream projects can expect plan validity under recomposition and may add +//! unit tests for built-in rules to guard against correctness regressions. +//! - Extensions should first try to address coverage gaps downstream for plan Review Comment: Maybe we can give specific guidance here -- "extending built in rules to match patterns that are not seen by other built in rules is discouraged" -- and offer a specific alternative "we suggest adding an custom optimizer rule to reform your plan into a standard shape that is recognized by the built in rules" ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,85 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending or replacing the default +//! list of optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! It's recommended to keep the default rule order and using supported configuration +//! options to disable optimizations. These variations preserve correctness. For +//! example, `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Custom pipelines +//! require more care, as described below. +//! +//! ## Custom Optimizer Pipelines +//! +//! The following extension pipeline can expect built-in optimizer rules to preserve +//! plan validity, but not to provide complete optimization coverage. +//! +//! ```text +//! // Custom rules +//! let rules = vec![ +//! rule3, +//! extension_rule_1, +//! rule1, +//! ]; +//! ``` +//! +//! - **Plan Validity:** Applying `rule1` after `rule3` should preserve query +//! semantics and return a valid plan without internal errors or invalid +//! plan shapes. +//! +//! The pipeline must still satisfy requirements needed for correctness. For +//! example, [`EnsureRequirements`] enforces input distribution and ordering. +//! Some constraints remain implicit or are documented only in individual rules. +//! +//! - **Optimization Coverage:** A non-default rule order may expose rules to +//! plan shapes they do not recognize, causing them to miss optimizations +//! that would apply in the default pipeline. +//! +//! For example, [`LimitedDistinctAggregation`] runs before [`EnsureRequirements`] +//! and [`CombinePartialFinalAggregate`], so it only targets the canonical shape. To keep +//! its implementation simple, it does not guarantee optimization coverage for +//! non-canonical shapes produced by extension pipelines. +//! +//! ```txt +//! -- Canonical (default pipeline): +//! -- Built-in rule recognize and optimize this shape +//! Limit +//! Aggregate(Final) +//! Aggregate(Partial) +//! +//! -- Non-canonical (repartitioning inserted first): +//! -- Built-in rule skip the optimization +//! Limit +//! Aggregate(Final) +//! Repartition +//! Aggregate(Partial) +//! ``` +//! +//! Contributions to DataFusion's built-in rules for custom optimizer pipelines +//! should follow these guidelines: +//! +//! - Downstream projects can expect plan validity under recomposition and may add Review Comment: I think this is a requirement, not a guideline Specifically, "reordering optimizer rules will still result in a valid plan" might be easier to udnerstand -- 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]
