ozankabak commented on code in PR #5661:
URL: https://github.com/apache/arrow-datafusion/pull/5661#discussion_r1148428682
##########
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:
We can also create new outright types instead of using aliases, as in:
```rust
struct Ordering(Vec<PhysicalSortExpr>);
struct OrderingRequirement(Vec<PhysicalSortRequirement>);
```
We would need some boilerplate to make them ergonomic when using elsewhere;
i.e.
```rust
impl Deref for Ordering {
type Target = Vec<PhysicalSortExpr>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl DerefMut for Ordering {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
impl Deref for OrderingRequirement {
type Target = Vec<PhysicalSortRequirement>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl DerefMut for OrderingRequirement {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
```
which will allow us to get rid of `make_sort_requirements_from_exprs` and
instead offer:
```rust
impl From<Ordering> for OrderingRequirement {
fn from(value: Ordering) -> Self {
OrderingRequirement(value.0.into_iter().map(|e| e.into()).collect())
// Couldn't get rid of the pesky ".0" for now!
}
}
```
If I get the spirit of your suggestion right, we can experiment with these
and get some sense of how involved such a refactor would be. I am not yet sure
about all the intricacies of the aliasing vs. tuple-struct trade-off, but we
would be happy to share our experiences/findings.
--
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]
