2010YOUY01 commented on code in PR #26351:
URL: https://github.com/apache/datafusion/pull/26351#discussion_r4311353050
##########
datafusion/physical-optimizer/src/optimizer.rs:
##########
@@ -81,7 +83,97 @@ impl PhysicalOptimizerContext for ConfigOnlyContext<'_> {
/// A rule-based physical optimizer.
///
-/// Applies its rule list in order in a single pass.
+/// Applies a *Pipeline* (an ordered list) of [`PhysicalOptimizerRule`]s in a
single pass.
+///
+/// # Physical Optimizer Contract
+///
+/// [`PhysicalOptimizer::new`] defines the default sequence of
+/// [`PhysicalOptimizerRule`]s. The optimizer applies them in order, in a
single
+/// pass. This section describes the contract between the optimizer rules, both
+/// for projects that extend or reorder the default rule list and for
+/// contributors adding new built-in rules.
+///
+/// ## Invariants
+///
+/// The following properties hold for all rules (built-in and custom):
+///
+/// 1. **Correctness.** The returned plan must a) be valid (it can be executed
+/// without internal errors) and b) produce the same results as the input
+/// plan. A rule may change the operators in the plan, such as changing
+/// which operator implements a join, but it may never change the query's
+/// semantics.
+///
+/// 2. **Reorderable.** Because each rule preserves correctness, running the
+/// rules in a different order must still yield a valid plan that produces
+/// the same results.
+///
+/// ### Exceptions
+///
+/// Some built-in rules do not yet fully satisfy the invariants above because
+/// they are responsible for making an initial plan *executable* rather than
for
+/// optimizing an existing plan:
+///
+/// - [`EnsureRequirements`] inserts the repartitioning and sorting needed to
+/// satisfy the distribution and ordering requirements.
+/// - [`OutputRequirements`] records output requirements.
+/// - [`SanityCheckPlan`] rejects plans whose requirements are not satisfied.
+/// It should run last.
+///
+/// Custom pipelines must keep these rules in a compatible relative order. We
+/// are working to remove these exceptions, so that all rules can be reordered
+/// freely.
+///
+/// ## Optimization Coverage Is Not Guaranteed
+///
+/// The invariants above guarantee *correctness*, not that every optimization
is
+/// applied. Built-in rules often match specific plan shapes that may only
occur
+/// at a certain point in the default pipeline. Adding, removing, or reordering
+/// rules can change plan shapes, so a built-in rule may silently miss an
+/// optimization that would apply in the default order. To keep their
+/// implementations simple, built-in rules typically only recognize shapes that
+/// the default pipeline produces.
+///
+/// For example, [`CombinePartialFinalAggregate`] runs after
+/// [`EnsureRequirements`] and only merges a `Partial` aggregate that is the
+/// direct child of a `Final` aggregate.
+///
+/// ```text
Review Comment:
I feel this example isn't a straightforward demonstration of canonical vs.
non-canonical forms, as it requires some knowledge of DataFusion-specific
implementation details.
It would be better to find a simpler example, though I can't think of one
yet. I'll update it when I come up with one.
--
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]
