jayzhan211 commented on code in PR #26107: URL: https://github.com/apache/datafusion/pull/26107#discussion_r4226060855
########## 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: Two pipelines that differ from the default order already exist, and the doc doesn't say whether either is supported (3.3 also rules out the first): 1. **Re-running the default sequence.** Ballista AQE re-runs the whole rule list after every stage. Before #22522, each run added another wrapper: `OutputRequirementExec → OutputRequirementExec → SortExec`. #22522 made `OutputRequirements` idempotent, with tests, solely for that. 2. **Appending.** `with_physical_optimizer_rule` adds the rule after `SanityCheckPlan`, so its output is never checked. With a rule that drops `SortExec`: ```sql SELECT a, ROW_NUMBER() OVER (ORDER BY a) AS rn FROM (VALUES (3), (1), (2)) AS t(a); ``` Inserted before `SanityCheckPlan`, it fails with `does not satisfy order requirements`. Appended, it silently returns `(3,1) (1,2) (2,3)`. -- 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]
