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 core::ops::Deref for Ordering {
type Target = Vec<PhysicalSortExpr>;
fn deref (self: &'_ Self) -> &'_ Self::Target {
&self.0
}
}
impl core::ops::DerefMut for Ordering {
fn deref_mut (self: &'_ mut Self) -> &'_ mut Self::Target {
&mut self.0
}
}
impl core::ops::Deref for OrderingRequirement {
type Target = Vec<PhysicalSortRequirement>;
fn deref (self: &'_ Self) -> &'_ Self::Target {
&self.0
}
}
impl core::ops::DerefMut for OrderingRequirement {
fn deref_mut (self: &'_ mut Self) -> &'_ mut Self::Target {
&mut self.0
}
}
```
which will allow us to avoid a public `make_sort_requirements_from_exprs`
and instead offer:
```rust
impl From<Ordering> for OrderingRequirement {
fn from(value: Ordering) -> Self {
OrderingRequirement(make_sort_requirements_from_exprs(&value))
}
}
```
If I get the spirit of your suggestion right, we can experiment with these
and incorporate such a change in our next commit addressing the feedback. We
can explore aliasing vs. tuple-struct methods and share our findings along with
the commit.
--
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]
