alamb commented on code in PR #10555: URL: https://github.com/apache/datafusion/pull/10555#discussion_r1605496677
########## datafusion/sql/src/unparser/expr.rs: ########## @@ -411,9 +411,34 @@ impl Unparser<'_> { Expr::Wildcard { qualifier: _ } => { not_impl_err!("Unsupported Expr conversion: {expr:?}") } - Expr::GroupingSet(_) => { - not_impl_err!("Unsupported Expr conversion: {expr:?}") - } + Expr::GroupingSet(grouping_set) => match grouping_set { + GroupingSet::GroupingSets(grouping_sets) => { + let expr_ast_sets = grouping_sets + .iter() + .map(|set| { + set.iter() + .map(|e| self.expr_to_sql(e)) + .collect::<Result<Vec<_>>>() + }) + .collect::<Result<Vec<_>>>()?; + + Ok(ast::Expr::GroupingSets(expr_ast_sets)) + } + GroupingSet::Cube(cube) => { + let expr_ast_sets = cube + .iter() + .map(|e| vec![self.expr_to_sql(e).unwrap()]) + .collect::<Vec<_>>(); Review Comment: I think it is important to not call `unwrap` here as it will panic You can collect the error with as you do above for grouping sets ```suggestion .map(|e| vec![self.expr_to_sql(e)]) .collect::<Result<Vec<_>>>()?; ``` (the same comment applies for `GroupingSet::Rollup`) -- 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-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org