alamb commented on code in PR #5867:
URL: https://github.com/apache/arrow-datafusion/pull/5867#discussion_r1157544792
##########
datafusion/physical-expr/src/sort_expr.rs:
##########
@@ -116,9 +116,11 @@ impl PhysicalSortRequirement {
}
}
-pub fn make_sort_requirements_from_exprs(
- ordering: &[PhysicalSortExpr],
-) -> Vec<PhysicalSortRequirement> {
+pub type ExprOrdering = Vec<PhysicalSortExpr>;
+pub type ExprOrderingRef<'a> = &'a [PhysicalSortExpr];
+pub type OrderingRequirement = Vec<PhysicalSortRequirement>;
Review Comment:
While these do simplify the signatures, I am a little worried about just
adding a type signature without additional supporting documentation.
If we are going to change the signatures again what do you think about going
all the way and actually making structures
```rust
struct ExprOrdering {
exprs: Vec<PhysicalSortExpr>;
}
```
And then make functions like (with documentation):
```rust
impl ExprOrdering {
fn new (exprs: Vec<PhysicalSortExpr>) -> Self {.._
fn from_requirements(requirements: OrderingRequirement -> Self {..}
}
```
For example, here is a PR that tries to encapsulate more of the
`PhysicalSortRequirements` along with a bunch of docs:
https://github.com/apache/arrow-datafusion/pull/5863
--
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]
