2010YOUY01 commented on code in PR #26107: URL: https://github.com/apache/datafusion/pull/26107#discussion_r4226709203
########## 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: Whether extra order is allowed is unspecified now, however allowing all possible composition is too permissive. It means optimizer rules can be arbitrarily reordered, repeated, added, or removed, and everything should still work. In practice, many rules rely on implicit assumptions about the surrounding pipeline. Ideally, we should explicitly define and enforce which compositions are valid. For example, a rule may be allowed to run multiple times, but must always run after rule X. Such constraints should be verifiable by the core, rather than relying on undocumented conventions. So I'm thinking a practical approach might be to first restrict it, and next add mechanisms to specify allowed extensions. The goal is not to eliminate extensibility, but to make its guarantees explicit and maintainable. -- 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]
