Skip to content
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions datafusion/physical-optimizer/src/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ( #25688 / #25572 from @zhuqi-lucas) as it has a different function than the other optimizer rules

//! 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a ticket that tracks this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm formulating this idea 😄

//! verify.
//!
//! 3. Extension rules should adapt to the built-in rules, not the other

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this -- we do aim to support arbitrary rules and I think many systems add their own custom rules already

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to propose a practical way to restrict it reasonably in

I think if we leave it unspecified, this assumption could be interpreted too broadly and eventually become a maintenance issue.

//! 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fix(physical-optimizer): make OutputRequirements idempotent #22522, each run added another wrapper: OutputRequirementExec → OutputRequirementExec → SortExec. fix(physical-optimizer): make OutputRequirements idempotent #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:
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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


use std::fmt::Debug;
use std::sync::Arc;
Expand Down
Loading