2010YOUY01 commented on PR #25554:
URL: https://github.com/apache/datafusion/pull/25554#issuecomment-6493482253
I have two major questions for this PR:
### Not sure this is a problem worth solving.
To eliminate one `RepartitionExec`, we have to write a query using
`COALESCE()`. This means applications need to be aware of this optimization and
structure their queries accordingly, which I think is unrealistic. More
realistically, this workload could be optimized at the application level, e.g.
by manually constructing the physical execution pipeline bypassing SQL.
```sql
EXPLAIN SELECT COALESCE(l.range_key, r.range_key) AS k, count(*),
sum(l.value)
FROM range_partitioned l
FULL JOIN range_partitioned_sparse r ON l.range_key = r.range_key
GROUP BY COALESCE(l.range_key, r.range_key);
```
### Plan property derivation is difficult to reason about.
`PlanProperties` serves as a cache of derived physical plan properties.
Conceptually, the dependency is:
```text
Operator configuration (e.g. join expressions/type) + child properties
---> PlanProperties
```
This PR's approach seems to introduce an additional dependency between
cached properties, specific to this optimization:
```text
HashJoinExec configuration + child properties
---> EquivalenceProperties
---> output Partitioning
```
This doesn't seem like the standard pattern, and might introduce subtle
bugs, such as order-dependent property derivation or cyclic dependencies in
other use cases.
### Conclusion
Given these concerns, I'd prefer to drop this optimization for now. However,
I'm happy to hear other perspectives.
--
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]
