[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5867: Use type aliases ExprOrdering and OrderingRequirements to simplify ordering-related type signatures

["alamb (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org