gabotechs commented on code in PR #18798:
URL: https://github.com/apache/datafusion/pull/18798#discussion_r2554867951
##########
datafusion/substrait/src/logical_plan/producer/rel/aggregate_rel.rs:
##########
@@ -91,10 +92,22 @@ pub fn to_substrait_groupings(
let groupings = match exprs.len() {
1 => match &exprs[0] {
Expr::GroupingSet(gs) => match gs {
- GroupingSet::Cube(_) => Err(DataFusionError::NotImplemented(
- "GroupingSet CUBE is not yet supported".to_string(),
- )),
- GroupingSet::GroupingSets(sets) => Ok(sets
+ GroupingSet::Cube(set) => {
+ // Generate power set of grouping expressions
+ let cube_sets = powerset_cloned(set)?;
+ cube_sets
+ .iter()
+ .map(|set| {
+ parse_flat_grouping_exprs(
+ producer,
+ set,
+ schema,
+ &mut ref_group_exprs,
+ )
+ })
+ .collect::<datafusion::common::Result<Vec<_>>>()
Review Comment:
Any opinions on just using the normal `powerset()` function here instead of
creating a `*_cloned()` variant? handling the clone here should be trivial:
```rust
- let cube_sets = powerset_cloned(set)?;
+ let cube_sets = powerset(set)?;
cube_sets
.iter()
.map(|set| {
parse_flat_grouping_exprs(
producer,
- set,
+ &set.iter().map(|v| (*v).clone()).collect::<Vec<_>>(),
schema,
&mut ref_group_exprs,
)
})
```
And that would avoid maintaining 3 function signatures (`powerset`,
`powerset_cloned` and `powerset_indices`)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
