Repository navigation
doc: Document the physical optimizer contract - #26351
2010YOUY01 wants to merge 3 commits into
Conversation
| //! example, [`EnsureRequirements`] enforces input distribution and ordering. | ||
| //! Some constraints remain implicit or are documented only in individual rules. | ||
| //! | ||
| //! - **Optimization Coverage:** A non-default rule order may expose rules to |
There was a problem hiding this comment.
A CI tool might help to verify optimization coverage is reachable from queries.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26351 +/- ##
==========================================
+ Coverage 82.83% 82.89% +0.06%
==========================================
Files 1151 1154 +3
Lines 455332 457620 +2288
Branches 455332 457620 +2288
==========================================
+ Hits 377165 379350 +2185
- Misses 55262 55263 +1
- Partials 22905 23007 +102 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @2010YOUY01 and @jayzhan211 and @zhuqi-lucas
I think this is better than what we current have, so I am approving it. However, I found it somewhat hard to understand as written and have a few suggestions on how to improve it.
It would also be fine with me to merge this PR in as is and I can make a follow on PR with a proposal to improve the wording
| //! disables the distinct aggregation soft-limit optimization. Custom pipelines | ||
| //! require more care, as described below. | ||
| //! | ||
| //! ## Custom Optimizer Pipelines |
There was a problem hiding this comment.
I found this somewhat confusingly worded. I would find it clearer if we started by stating the invariants:
- All optimizer rules must preserve plan correctness (the output plan can be run and produces the same results as the input plan). We can note there are some built in plans that aren't yet there (like EnforceRequirements)
- That reordering optimizer rules (other than the exceptions) should result in a plan that produces the same results
Then we could then move on to the explicit warning that adding / reordering rules may make some of the built in rules less effective as some built in rules (e.g. CombinePartialFinalAggregate) match certain plan shapes.
p.s. I would love to split out EnforceRequrements into its own Analyzer thing precisely so we don't have to have to mention the special case for EnforceRequirements above
There was a problem hiding this comment.
Thank you for the review! I've addressed other feedbacks since they're smaller wording improvements, but not this one.
I think i don't fully get how you want to express it better, feel free to directly push changes, or do a follow-up PR 🙏🏼
5 participants
Which issue does this PR close?
Rationale for this change
See issue for rationale, this PR documents what has been agreed on
What changes are included in this PR?
doc only
What is the testing strategy for this PR?
Are there any user-facing changes?