Repository navigation
docs: Improve optimizer rule reference by consolidating into rustdocs - #26899
Conversation
…own file Move the built-in analyzer, logical optimizer, and physical optimizer rule tables from `optimizer_rule_reference.md` into the doc comments on `Analyzer::new`, `Optimizer::new`, and `PhysicalOptimizer::new`, next to the rule lists they describe. Link to those from the `datafusion` crate docs. Add struct-level docs to `UnionsToFilter` and `EliminateCrossJoin`, which had none, and remove the markdown file along with the test module that parsed it. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…r-rule-reference # Conflicts: # datafusion/core/src/optimizer_rule_reference.md
Drop the `order` and `phase` columns (row position gives the order, and the phase now sits in the summary), tighten the preambles, and shorten each rule summary. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Define the `Analyzer`, `Optimizer` and `PhysicalOptimizer` intra-doc links used by the crate overview, tidy the wording, and point the `Analyzer` and `Optimizer` struct docs at `new` where the built-in rules are listed. Co-Authored-By: Claude Fable 5.1 <[email protected]>
| //! | ||
| //! 1. The [`LogicalPlan`] is checked and rewritten to enforce | ||
| //! semantic rules, such as type coercion, by [`AnalyzerRule`]s | ||
| //! 1. The [`LogicalPlan`] is checked and rewritten by the [`Analyzer`], using |
There was a problem hiding this comment.
I tried to make this introduction clear and provide links to the structures with more detailed documentation for each feature
| @@ -1,72 +0,0 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
this test existed to ensure the order matches the documentation, but I think now that the list of rules is in the same file as the actual lists it will be much easier (and obvious) that we need to update the docs when we update the actual list
There was a problem hiding this comment.
if this isn't adequate, we can perhaps make individual unit tests in each module that verify the order is correct, rather than something that does it across multiple modules
alamb
changed the title
|
@jayzhan211 and @2010YOUY01 and @zhuqi-lucas I wonder what you think about this PR? |
zhuqi-lucas
left a comment
zhuqi-lucas
left a comment
There was a problem hiding this comment.
Checked the three tables against Analyzer::new(), Optimizer::new() and PhysicalOptimizer::new() on this branch: the order and the EXPLAIN names all match (including LimitAggregation for TopKAggregation and FilterPushdown(Post)). Looks good.
| /// Applies rules in order for up to `datafusion.optimizer.max_passes` passes, | ||
| /// stopping early if the plan repeats. | ||
| /// | ||
| /// # Built-in Optimizer Rules |
There was a problem hiding this comment.
The deleted optimizer_rule_reference.rs was what failed CI when a rule was added or reordered without updating the docs. A small test could keep that guard here, e.g. include_str!("optimizer.rs"), parse the rows under this heading and compare them with Optimizer::new().rules names (same for the analyzer and physical optimizer). Fine as a follow-up.
| /// The rules are applied in the following order. Note: the list may change | ||
| /// between releases. | ||
| /// | ||
| /// | rule | summary | |
There was a problem hiding this comment.
Nit: unlike the other two tables, this one has no "name in EXPLAIN plan" column (resolve_grouping_function, type_coercion).
|
Thank you @zhuqi-lucas -- I have made the changes you suggested |
Which issue does this PR close?
Rationale for this change
datafusion/core/src/optimizer_rule_reference.mddocuments the optimizer and rulesIt was added in this PR from @kumarUjjawal
While it is spliced into the
datafusioncrate docs viainclude_str!, keeping the refernece a separate markdown file meanseliminate_joinsummary no longer matched the rule's behavior)What changes are included in this PR?
This PR moves the content next to the code so the rule list and its documentation
What is the testing strategy for this PR?
CI
Are there any user-facing changes?
No API changes. The rule reference moves from the
datafusioncrate root docs to the three constructor docs.