alamb commented on code in PR #5867:
URL: https://github.com/apache/arrow-datafusion/pull/5867#discussion_r1158478086
##########
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:
> I tried a refactor using full-fledged types but the resulting changes were
more involved/time-consuming, so I opted to go with aliases until we know there
are concrete use cases requiring multiple attributes in types such as
ExprOrdering.
This makes sense -- thank you for trying. It definitely took me far more
time to make https://github.com/apache/arrow-datafusion/pull/5863 than I
expected -- mostly to come up with coherent description of what the structures
did.
> We can just merge https://github.com/apache/arrow-datafusion/pull/5863 and
defer/revisit this kind of a refactor when we have a real use case for multiple
fields in types such as ExprOrdering.
I think this is my preference for the moment. Let's wait to see if anyone
else has comments
--
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]
