alamb commented on code in PR #26107: URL: https://github.com/apache/datafusion/pull/26107#discussion_r4233603238
########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by Review Comment: I think this is overly pessimistic. I think the only rules that have correctness requirements are `OutputRequirements` and `EnforceRequirements`. Other rules may make assumptions about plan shape for performance but not for correctness that I understand. This is part of the reason i would like to treat `EnforceRequirements` specially as an `Analyzer` ( https://github.com/apache/datafusion/pull/25688 / https://github.com/apache/datafusion/issues/25572 from @zhuqi-lucas) as it has a different function than the other optimizer rules ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by +//! earlier rules. Correctness is only guaranteed in the default order. +//! +//! 2. **Use configuration to disable optimizations.** Configuration options +//! provide supported variations of the default pipeline that preserve +//! correctness. For example, +//! `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Removing rules +//! directly from the pipeline may produce invalid plans. +//! +//! 3. **Adding optimizer rules.** +//! +//! 1. Rules added within DataFusion or downstream must respect the +//! assumptions of the surrounding rules. Many of these are implicit or +//! documented only in individual rules. Changes to the default pipeline +//! may require updates to rules that rely on them. +//! +//! 2. DataFusion aims to make these assumptions easier to understand and +//! verify. +//! +//! 3. Extension rules should adapt to the built-in rules, not the other +//! way around. DataFusion does not aim to support arbitrary downstream +//! pipelines such as: +//! +//! ```text +//! // Potential downstream usage: +//! // +//! // Reordered default rules mixed with extension rules +//! let rules = vec![ +//! rule3, +//! extension_rule1, +//! rule1, +//! // ... +//! ]; +//! ``` +//! +//! Do not extend built-in rules or add unit tests within DataFusion +//! solely to support such downstream pipelines. Review Comment: > 2\. **Appending.** `with_physical_optimizer_rule` adds the rule after `SanityCheckPlan`, so its output is never checked. With a rule that drops `SortExec`: This is a good reason in my mind to remove `SanityCheckPlan` as an OptimizerPass and always run it after the optimizer is done. maybe we can file a ticket / PR to do so ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by +//! earlier rules. Correctness is only guaranteed in the default order. +//! +//! 2. **Use configuration to disable optimizations.** Configuration options +//! provide supported variations of the default pipeline that preserve +//! correctness. For example, +//! `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Removing rules +//! directly from the pipeline may produce invalid plans. +//! +//! 3. **Adding optimizer rules.** +//! +//! 1. Rules added within DataFusion or downstream must respect the +//! assumptions of the surrounding rules. Many of these are implicit or +//! documented only in individual rules. Changes to the default pipeline +//! may require updates to rules that rely on them. +//! +//! 2. DataFusion aims to make these assumptions easier to understand and +//! verify. +//! +//! 3. Extension rules should adapt to the built-in rules, not the other +//! way around. DataFusion does not aim to support arbitrary downstream +//! pipelines such as: +//! +//! ```text +//! // Potential downstream usage: +//! // +//! // Reordered default rules mixed with extension rules +//! let rules = vec![ +//! rule3, +//! extension_rule1, +//! rule1, +//! // ... +//! ]; +//! ``` +//! +//! Do not extend built-in rules or add unit tests within DataFusion +//! solely to support such downstream pipelines. Review Comment: > It means optimizer rules can be arbitrarily reordered, repeated, added, or removed, and everything should still work. Other than `EnforceRequirements` I think this is true today, as long as you define "everything should still work" as "the plan generates correct output answers". > In practice, many rules rely on implicit assumptions about the surrounding pipeline. This is true for a bunch of the optimizations -- they look for specific plan patterns created by prior optimizer passes. However I don't think they rely on specific plan patters for generating correct answers ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by +//! earlier rules. Correctness is only guaranteed in the default order. +//! +//! 2. **Use configuration to disable optimizations.** Configuration options +//! provide supported variations of the default pipeline that preserve +//! correctness. For example, +//! `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Removing rules +//! directly from the pipeline may produce invalid plans. +//! +//! 3. **Adding optimizer rules.** +//! +//! 1. Rules added within DataFusion or downstream must respect the +//! assumptions of the surrounding rules. Many of these are implicit or +//! documented only in individual rules. Changes to the default pipeline +//! may require updates to rules that rely on them. +//! +//! 2. DataFusion aims to make these assumptions easier to understand and Review Comment: Do we have a ticket that tracks this? ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by +//! earlier rules. Correctness is only guaranteed in the default order. +//! +//! 2. **Use configuration to disable optimizations.** Configuration options +//! provide supported variations of the default pipeline that preserve +//! correctness. For example, +//! `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Removing rules +//! directly from the pipeline may produce invalid plans. +//! +//! 3. **Adding optimizer rules.** +//! +//! 1. Rules added within DataFusion or downstream must respect the +//! assumptions of the surrounding rules. Many of these are implicit or +//! documented only in individual rules. Changes to the default pipeline +//! may require updates to rules that rely on them. +//! +//! 2. DataFusion aims to make these assumptions easier to understand and +//! verify. +//! +//! 3. Extension rules should adapt to the built-in rules, not the other Review Comment: I disagree with this -- we do aim to support arbitrary rules and I think many systems add their own custom rules already ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -16,6 +16,63 @@ // under the License. //! Physical optimizer traits +//! +//! # Physical Optimizer Contract +//! +//! This section explains the contract for extending the default list of +//! optimizer rules. +//! +//! [`PhysicalOptimizer::new`] defines the default rule sequence: +//! +//! ```text +//! // Default rules +//! let rules = vec![ +//! rule1, +//! rule2, +//! rule3, +//! rule4, +//! // ... +//! ]; +//! ``` +//! +//! 1. **Keep the default order.** Rules may rely on properties established by +//! earlier rules. Correctness is only guaranteed in the default order. +//! +//! 2. **Use configuration to disable optimizations.** Configuration options +//! provide supported variations of the default pipeline that preserve +//! correctness. For example, +//! `SET datafusion.optimizer.enable_distinct_aggregation_soft_limit = false` +//! disables the distinct aggregation soft-limit optimization. Removing rules +//! directly from the pipeline may produce invalid plans. +//! +//! 3. **Adding optimizer rules.** +//! +//! 1. Rules added within DataFusion or downstream must respect the +//! assumptions of the surrounding rules. Many of these are implicit or +//! documented only in individual rules. Changes to the default pipeline +//! may require updates to rules that rely on them. +//! +//! 2. DataFusion aims to make these assumptions easier to understand and +//! verify. +//! +//! 3. Extension rules should adapt to the built-in rules, not the other +//! way around. DataFusion does not aim to support arbitrary downstream +//! pipelines such as: +//! +//! ```text +//! // Potential downstream usage: +//! // +//! // Reordered default rules mixed with extension rules +//! let rules = vec![ +//! rule3, +//! extension_rule1, +//! rule1, +//! // ... +//! ]; +//! ``` +//! +//! Do not extend built-in rules or add unit tests within DataFusion +//! solely to support such downstream pipelines. Review Comment: > Re-running the default sequence. Ballista AQE re-runs the whole rule list after every stage. Our system in InfluxDB3 also runs quite a few custom optimizers -- 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]
