ozankabak commented on code in PR #5661:
URL: https://github.com/apache/arrow-datafusion/pull/5661#discussion_r1148459560
##########
datafusion/core/src/physical_plan/joins/symmetric_hash_join.rs:
##########
@@ -399,10 +393,14 @@ impl ExecutionPlan for SymmetricHashJoinExec {
self.schema.clone()
}
- fn required_input_ordering(&self) -> Vec<Option<&[PhysicalSortExpr]>> {
+ fn required_input_ordering(&self) ->
Vec<Option<Vec<PhysicalSortRequirement>>> {
Review Comment:
OK, having played with these ideas for 2-3 hours, here are my observations:
Changing the return types of `output_ordering` and `required_input_ordering`
with non-plug-in-compatible types are really challenging. I made two refactor
attempts: One with a new `struct` and one with a tuple-struct like my previous
example. I couldn't finish either of them in 2-3 hours -- it turns out doing
this results in massive changes and things are quite intertwined.
However, I have some good news too: We can achieve an almost-as-good
readability level with type aliasing:
```rust
pub type ExprOrdering = Vec<PhysicalSortExpr>;
pub type ExprOrderingRef<'a> = &'a [PhysicalSortExpr];
pub type OrderingRequirement = Vec<PhysicalSortRequirement>;
```
With these "types", I was able complete the refactor and significantly
simplify how the code reads; i.e.
```rust
impl ExecutionPlan {
fn required_input_ordering(&self) -> Vec<Option<OrderingRequirement>>;
fn output_ordering(&self) -> Option<ExprOrderingRef>;
}
```
with `ExprOrdering` replacing `Vec<PhysicalSortExpr>` where the actual
vector is required.
This refactor is not large in terms of LOCs (~250 lines change, almost
entirely consisting of simple substitutions), but it touches almost 50 files!
So we may want to leave it to a follow-on too -- I am fine with either.
--
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]
