You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "2010YOUY01 (via GitHub)" <gi...@apache.org> on 2026/10/07 08:58:21 UTC
[PR] doc: Document the physical optimizer contract [datafusion]
2010YOUY01 opened a new pull request, #26107:
URL: https://github.com/apache/datafusion/pull/26107
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
Please explain the problem you are trying to solve in terms of the user-visible
behavior, rather than the implementation.
For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the
implementation. "COUNT(DISTINCT) returns wrong results when the column contains
nulls" is the user-visible problem.
-->
This PR tries to captures the current state and explains it. Please let me know if I’ve missed anything.
## What changes are included in this PR?
docs only
<!--
There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## What is the testing strategy for this PR?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'.
If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it.
You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised.
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26107:
URL: https://github.com/apache/datafusion/pull/26107#issuecomment-6035227706
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26107?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.72%. Comparing base ([`7b1e55d`](https://app.codecov.io/gh/apache/datafusion/commit/7b1e55d4c2291acf6b9e7eb20dcdc62f25a8fe98?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`833aebd`](https://app.codecov.io/gh/apache/datafusion/commit/833aebd97395605228da219666a1329e7da5c76d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26107 +/- ##
==========================================
- Coverage 82.72% 82.72% -0.01%
==========================================
Files 1147 1147
Lines 449175 449213 +38
Branches 449175 449213 +38
==========================================
+ Hits 371598 371627 +29
- Misses 54929 54931 +2
- Partials 22648 22655 +7
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26107?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #26107:
URL: https://github.com/apache/datafusion/pull/26107#discussion_r4235966562
##########
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:
I'm formulating this idea 😄
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #26107:
URL: https://github.com/apache/datafusion/pull/26107#discussion_r4242435689
##########
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 am still not sure it is a good idea to add additional restrictions on semantics between optimizer passes. I think the semantics of what a ExecutionPlan needs to produce correct output should stay within the operator itself (e.g. a try_new method that verifies it).
I am aligned / agree with your point 3 on https://github.com/apache/datafusion/issues/26171 👍
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #26107:
URL: https://github.com/apache/datafusion/pull/26107#issuecomment-6092570403
Thank you for the feedbacks @alamb and @jayzhan211 , I also have some great conversation with @zhuqi-lucas on a similar topic while reviewing https://github.com/apache/datafusion/pull/23599
I'd love to move the conversation into the below issue, I tried to explain a less restrictive and practical middle ground:
- https://github.com/apache/datafusion/issues/26171
The key points are examples in point 2, and proposed contract in point 3
I'll rework this PR after reaching some agreements there.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #26107:
URL: https://github.com/apache/datafusion/pull/26107#discussion_r4235964676
##########
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 tried to propose a practical way to restrict it reasonably in
- (see point 3) https://github.com/apache/datafusion/issues/26171
I think if we leave it unspecified, this assumption could be interpreted too broadly and eventually become a maintenance issue.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] doc: Document the physical optimizer contract [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org