[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5559: improve: support combining multiple grouping expressions

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

alamb commented on code in PR #5559:
URL: https://github.com/apache/arrow-datafusion/pull/5559#discussion_r1135522137


##########
datafusion/expr/src/utils.rs:
##########
@@ -69,6 +69,94 @@ pub fn grouping_set_expr_count(group_expr: &[Expr]) -> Result<usize> {
     }
 }
 
+fn powerset<T>(slice: &[T]) -> Vec<Vec<&T>> {
+    let mut v = Vec::new();
+    for mask in 0..(1 << slice.len()) {
+        let mut ss = vec![];
+        let mut bitset = mask;
+        while bitset > 0 {
+            let rightmost: u64 = bitset & !(bitset - 1);
+            let idx = rightmost.trailing_zeros();
+            let item = slice.get(idx as usize).unwrap();
+            ss.push(item);
+            // zero the trailing bit
+            bitset &= bitset - 1;
+        }
+        v.push(ss);
+    }
+    v
+}
+
+/// Convert multiple multiple grouping expressions into one [`GroupingSet::GroupingSets`],\

Review Comment:
   ```suggestion
   /// Convert multiple grouping expressions into one [`GroupingSet::GroupingSets`],\
   ```



##########
datafusion/expr/src/utils.rs:
##########
@@ -69,6 +69,94 @@ pub fn grouping_set_expr_count(group_expr: &[Expr]) -> Result<usize> {
     }
 }
 
+fn powerset<T>(slice: &[T]) -> Vec<Vec<&T>> {

Review Comment:
   Can you please add a docstring explaining what the expected output of this function is intended to be?
   
   For example, is it https://en.wikipedia.org/wiki/Power_set?



##########
datafusion/expr/src/utils.rs:
##########
@@ -1288,4 +1379,122 @@ mod tests {
         }
         Ok(())
     }
+
+    #[test]
+    fn test_enumerate_grouping_sets() {
+        let milti_cols = vec![col("col1"), col("col2"), col("col3")];

Review Comment:
   Minor typo here `milti` --> `multi`



##########
datafusion/expr/src/utils.rs:
##########
@@ -69,6 +69,94 @@ pub fn grouping_set_expr_count(group_expr: &[Expr]) -> Result<usize> {
     }
 }
 
+fn powerset<T>(slice: &[T]) -> Vec<Vec<&T>> {
+    let mut v = Vec::new();
+    for mask in 0..(1 << slice.len()) {

Review Comment:
   I think that this should verify that that slice.len() is less than 64 and return an error  to avoid overflow / panic?



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