2010YOUY01 commented on PR #25688:
URL: https://github.com/apache/datafusion/pull/25688#issuecomment-6008341569
> As neat an idea of having multiple classes of constraints that can be
satisfied at different points, I think it makes it harder to reason about the
optimizer as a whole and any OptimizerRule individually -- we would need some
way for each rule to communicate what its expected input and output boundary was
If I understand the assumptions behind the optimizer implementation
correctly, adding more explicit constraints should make both understanding and
implementation easier.
Let me first restate the assumptions. I may be missing something here since
I have less experience working on optimizers.
```txt
# Assumptions for the physical optimizer
DataFusion provides a default ordered list of optimizer rules.
// Default rules
rules = [
rule1,
rule2,
rule3,
rule4,
...
]
- The default rules are only guaranteed to be safe when run in the
prescribed order.
- To disable specific behavior, use configuration options that define
known-safe variations. For example,
`set datafusion.optimizer.enable_distinct_aggregation_soft_limit = false`
disables one specific aggregation optimization without arbitrarily
modifying the rule pipeline.
- (This sounds intimidating, but seems close to the current state):
To implement an extension rule outside core, you need to understand the
implicit constraints established by the surrounding default optimizer rules.
Otherwise, changes in the default rules might break the plan.
Currently, many of these constraints are only documented locally inside
individual rules.
# Future improvement direction
Make these constraints easier to understand, express, and test.
# Not intended usage
Arbitrarily reorder/remove default rules downstream and expect the resulting
optimizer pipeline to remain valid.
// Reordered default rules mixed with extension rules
rules = [
rule3,
extension_rule1,
rule1,
...
]
```
## More constraints can simplify optimizer extensions
If we explicitly exclude arbitrary reordering of the default optimizer rule
list from the supported use case, then stronger constraints can simplify the
optimizer:
- **Simpler implementation:** each optimizer rule needs to handle fewer
possible input states if its preconditions are known.
- **Safer composition:** a rule knows which invariants it must preserve so
that later rules continue to work correctly.
### Walkthrough
In @zhuqi-lucas's use case:
```txt
rules = [
rule1,
rule2,
EnsureRequirements,
rule3,
extension_rule,
...
]
```
If we treat the analyzer/optimizer split as a general boundary, the
preconditions and postconditions of `extension_rule` become much clearer:
- **Assumption:** input plans already satisfy their distribution/ordering
requirements (`RepartitionExec`s have been inserted and the plan is runnable).
- **Promise:** the rule must not invalidate those requirements. It either
preserves the existing enforcement operators or re-runs `EnsureRequirements`
internally when necessary.
The simplification provided by this boundary is that a semantically
equivalent plan has a canonical physical shape at this point in the optimizer.
```txt
// plan1 and plan2 are semantically equivalent, but the analyzer boundary
// guarantees that this rule only needs to handle plan1.
//
// This reduces the number of physical shapes the rule needs to reason about.
// plan1
AggregateExec(mode=final)
--RepartitionExec
----AggregateExec(mode=partial)
// plan2
AggregateExec(mode=final)
--AggregateExec(mode=partial)
```
More constraints can reduce the problem space further. For example, if this
extension rule needs to inspect projections, its implementation becomes simpler
if projected plans also have one canonical shape at this boundary, rather than
allowing several equivalent representations.
## Proceeding with this PR
Now @zhuqi-lucas and @alamb seem more in favor of implementing the
analyzer/optimizer split first, and treating a more general boundary mechanism
as a follow-up.
My current preference is to implement the more general optimizer boundary
directly, to avoid introducing multiple mechanisms for the same underlying
problem. That said, if I have misunderstood the assumptions above, this
proposal would definitely need to be reworked.
I'd like to hear your thoughts before deciding on the next step. If you also
think a general boundary mechanism is a plausible direction, I can put together
a PoC this week.
--
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]
