[GitHub] [calcite] julianhyde commented on a change in pull request #2464: [CALCITE-4700] AggregateUnionTransposeRule produces wrong group sets for the top Aggregate (Vladimir Ozerov)

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

julianhyde commented on a change in pull request #2464:
URL: https://github.com/apache/calcite/pull/2464#discussion_r675866556



##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -151,9 +154,36 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
 
     // create a new union whose children are the aggregates created above
     relBuilder.union(true, union.getInputs().size());
+
+    // Create the top aggregate. We must adjust group key indexes of the
+    // original aggregate. E.g., if the original tree was:
+    //
+    // Aggregate[groupSet=$1, ...]
+    //   Union[...]
+    //
+    // Then the new tree should be:
+    // Aggregate[groupSet=$0, ...]
+    //   Union[...]
+    //     Aggregate[groupSet=$1, ...]
+    Mapping topGroupMapping = Mappings.create(MappingType.INVERSE_SURJECTION,
+        union.getRowType().getFieldCount(),
+        aggRel.getGroupCount()
+    );

Review comment:
       close parenthesis should be on same line, not new line

##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -151,9 +154,36 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
 
     // create a new union whose children are the aggregates created above
     relBuilder.union(true, union.getInputs().size());
+
+    // Create the top aggregate. We must adjust group key indexes of the
+    // original aggregate. E.g., if the original tree was:
+    //
+    // Aggregate[groupSet=$1, ...]
+    //   Union[...]
+    //
+    // Then the new tree should be:
+    // Aggregate[groupSet=$0, ...]
+    //   Union[...]
+    //     Aggregate[groupSet=$1, ...]
+    Mapping topGroupMapping = Mappings.create(MappingType.INVERSE_SURJECTION,
+        union.getRowType().getFieldCount(),
+        aggRel.getGroupCount()
+    );
+    int i = 0;
+    for (int groupKey : aggRel.getGroupSet()) {
+      topGroupMapping.set(groupKey, i++);
+    }
+
+    ImmutableBitSet topGroupSet = Mappings.apply(topGroupMapping, aggRel.getGroupSet());
+    ImmutableList.Builder<ImmutableBitSet> builder = ImmutableList.builder();
+    for (ImmutableBitSet groupSet : aggRel.getGroupSets()) {
+      builder.add(Mappings.apply(topGroupMapping, groupSet));
+    }
+    ImmutableList<ImmutableBitSet> topGroupSets = builder.build();

Review comment:
       Consider using `Mappings.apply2(Mapping, Iterable<ImmutableBitSet>)`; see how that method is used in `AggregateJoinTransposeRule`.

##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java
##########
@@ -151,9 +154,36 @@ public AggregateUnionTransposeRule(Class<? extends Aggregate> aggregateClass,
 
     // create a new union whose children are the aggregates created above
     relBuilder.union(true, union.getInputs().size());
+
+    // Create the top aggregate. We must adjust group key indexes of the
+    // original aggregate. E.g., if the original tree was:
+    //
+    // Aggregate[groupSet=$1, ...]
+    //   Union[...]
+    //
+    // Then the new tree should be:
+    // Aggregate[groupSet=$0, ...]
+    //   Union[...]
+    //     Aggregate[groupSet=$1, ...]
+    Mapping topGroupMapping = Mappings.create(MappingType.INVERSE_SURJECTION,
+        union.getRowType().getFieldCount(),
+        aggRel.getGroupCount()
+    );
+    int i = 0;
+    for (int groupKey : aggRel.getGroupSet()) {
+      topGroupMapping.set(groupKey, i++);
+    }

Review comment:
       Consider using `ImmutableBitSet.nth` rather than mutable `i`; it may or may not be more concise, but a few other rules have to build this same mapping.




-- 
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: commits-unsubscribe@calcite.apache.org

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