2010YOUY01 commented on PR #25688:
URL: https://github.com/apache/datafusion/pull/25688#issuecomment-5965009472
Here is what I understand from the latest discussion and code diff. I used
AI to help summarize it, so please point out anything I misunderstood.
The latest change keeps the existing physical optimizer rule order, but
introduces one hard boundary through the `PhysicalAnalyzer` /
`PhysicalOptimizer` split.
Conceptually:
- **Before the boundary:** there are no enforced repartition/sort operators
yet, so rules are relatively free to transform the plan.
- **After the boundary:** repartition/sort operators may already exist, so
every later rule must recognize and preserve the distribution and ordering
requirements that have already been satisfied.
My major suggestion is to make this boundary mechanism **extensible**,
instead of encoding one specific boundary through the Analyzer/Optimizer split.
```text
# Current design
PhysicalAnalyzer:
OutputRequirements(add)
aggregate_statistics
join_selection
LimitedDistinctAggregation
FilterPushdown
WindowTopN
EnsureRequirements
# Boundary:
# distribution and ordering requirements are satisfied
PhysicalOptimizer:
...remaining existing rules...
# What I mean by extensible boundaries
rule1
rule2
rule3
# Boundary A:
# distribution and ordering requirements are now enforced
rule4
rule5
# Boundary B:
# another optimizer invariant is now established
rule6
rule7
```
If these boundaries can be represented explicitly, I think optimizer
maintenance becomes much easier. Rules can clearly see which assumptions they
are allowed to rely on, instead of those assumptions being hidden in rule
ordering and implementation details.
Projection pushdown is one concrete example of where another boundary could
be useful. The important part is not this specific invariant, but that the same
mechanism could express it naturally:
```text
rule1
# Before this point, projection has one canonical representation:
#
# ProjectionExec
# -- child
ProjectionPushDown
# After this point, projection may either stay explicit:
#
# ProjectionExec(c1 + c2)
# -- Join(output = [c1, c2])
#
# or be fused into an operator that supports it:
#
# Join(
# output = [c1, c2],
# projection = [c1],
# )
rule2
rule3
```
I suspect there are more hidden invariants like this in the optimizer today,
so I think it is important that the mechanism can naturally support multiple
boundaries rather than only this one distribution/ordering boundary.
One additional nice-to-have is validation. I don't think this needs to be a
major design decision, since it seems relatively easy to extend once boundaries
are represented explicitly.
For example, after a boundary establishes an invariant, we could validate it
after every subsequent rule, at least in tests or debug builds:
```text
EnsureRequirements
Rule3
ValidateEnsureRequirements
Rule4
ValidateEnsureRequirements
```
That would make it much easier to identify exactly which rule first breaks
the invariant, rather than discovering the violation only after the entire
optimizer finishes.
--
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]
