[GitHub] [arrow-datafusion] andygrove commented on a diff in pull request #3286: Add `Aggregate::try new` with validation checks

[GitBox <gi...@apache.org>]

andygrove commented on code in PR #3286:
URL: https://github.com/apache/arrow-datafusion/pull/3286#discussion_r957636257


##########
datafusion/expr/src/utils.rs:
##########
@@ -45,6 +45,22 @@ pub fn exprlist_to_columns(expr: &[Expr], accum: &mut HashSet<Column>) -> Result
     Ok(())
 }
 
+/// Count the number of distinct exprs in a list of group by expressions. If the
+/// first element is a `GroupingSet` expression then it must be the only expr.
+pub fn grouping_set_expr_count(group_expr: &[Expr]) -> Result<usize> {
+    if let Some(Expr::GroupingSet(grouping_set)) = group_expr.first() {

Review Comment:
   I also found this frustrating to work with. Maybe we could introduce another enum?
   
   ```
   pub enum Grouping {
       Expr(Vec<Expr>),
       Set(GroupingSet)
   }
   ```
   
   and then
   
   ```
   pub struct Aggregate {
       pub input: Arc<LogicalPlan>,
       pub group_expr: Grouping,
       pub aggr_expr: Vec<Expr>,
       pub schema: DFSchemaRef,
   }
   ```



-- 
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