You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/01/14 05:18:04 UTC
[GitHub] [calcite] DonnyZone opened a new pull request #1756: [CALCITE-1824]
GROUP_ID returns wrong result
DonnyZone opened a new pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756
See the discussion in [CALCITE-1824](https://issues.apache.org/jira/browse/CALCITE-1824), we rewrite the query that contains `GROUP_ID` as `UNION-ALL`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366178014
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
+ }
+ }
+ final RelNode project = relBuilder.project(selectList)
+ .rename(selectListNames).build();
Review comment:
Should `project(selectList, selectListNames)` be used instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366202853
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
They are duplicate group set before transformation into to canonical form `grouping sets (a, b, c)`. The `UnionAll`'s children and corresponding `group_id` is:
```
Union All
grouping sets(a, b, c) --- 0
grouping sets(a, c) --- 1
grouping sets(c) --- 2
grouping sets(c) --- 3
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366407494
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
For the values in map, we do not care about whether they are sorted. But the flatten group sets (i.e.,the map's keyset) should be ordered when constructing Resolved.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366214662
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -4488,6 +4488,37 @@ private void startOfGroupStep3(String startOfGroup) {
"empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>. */
Review comment:
Would you please remove useless `test case for...` and replace it with something like as follows?
Rewrites `group_id() ... group by grouping sets (deptno, deptno, deptno, (), ())` into
```
select 0 ... group by grouping sets (deptno, ())
union all
select 1 ... group by grouping sets (deptno, ())
union all
select 2 ... group by deptno
```
?
The reason is `EnumerableAggregate(group=[{7}], groups=[[{7}, {}]]` is hard to understand, so the current test requires understanding of both `grouping sets` and `EnumerableAggregate`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366247257
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
`SortedMap`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366203189
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
+ }
+ }
+ final RelNode project = relBuilder.project(selectList)
+ .rename(selectListNames).build();
+ projects.add(project);
+ }
+ if (projects.size() == 1) {
+ return projects.get(0);
+ }
+ return LogicalUnion.create(projects, true);
+ }
+
+ /**
+ * Add a group set to the group sets of a specific group id
+ */
+ private void addGroupSet(int groupId, ImmutableBitSet groupSet,
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets) {
+ final Set<ImmutableBitSet> groupSets = groupIdToGroupSets.getOrDefault(
+ groupId, Sets.newTreeSet(ImmutableBitSet.COMPARATOR));
Review comment:
Yes, it can be removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366178730
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -5335,6 +5443,11 @@ public RexNode lookupAggregates(SqlCall call) {
return aggCalls;
}
+ boolean containsGroupId() {
+ return aggCalls.stream().anyMatch(
+ agg -> agg.getAggregation().kind == SqlKind.GROUP_ID);
Review comment:
private?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366407494
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
For the entries in `map`, we do not care about whether they are sorted. But the flatten group sets (i.e.,the map's keyset) should be ordered when constructing `Resolved`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366215049
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -110,8 +111,11 @@ private Resolved resolve() {
final Set<ImmutableBitSet> flatGroupSets =
Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final Map<ImmutableBitSet, Integer> groupSetCount = new HashMap<>();
for (List<ImmutableBitSet> groupSet : Linq4j.product(builder.build())) {
- flatGroupSets.add(ImmutableBitSet.union(groupSet));
+ final ImmutableBitSet set = ImmutableBitSet.union(groupSet);
+ groupSetCount.put(set, groupSetCount.getOrDefault(set, 0) + 1);
+ flatGroupSets.add(set);
Review comment:
Is `flatGroupSets` still valid? `groupSetCount` and `flatGroupSets` seem to duplicate each other.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366263800
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
Isn't a regular `Map` sufficient here by the way?
It does look weird that a `SortedMap` is passed as a `Map` parameter, so the order of the entries is lost.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366179020
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -4488,6 +4488,37 @@ private void startOfGroupStep3(String startOfGroup) {
"empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>. */
+ @Test public void testGroupId() {
+ CalciteAssert.that()
+ .with(CalciteAssert.Config.SCOTT)
+ .query("select deptno, group_id() + 1 as g, count(*) as c\n"
+ + "from \"scott\".emp\n"
+ + "group by grouping sets (deptno, deptno, deptno, (), ())\n"
+ + "having group_id() > 0")
+ .explainContains("EnumerableCalc(expr#0..2=[{inputs}], expr#3=[1], expr#4=[+($t1, $t3)], "
+ + "expr#5=[0], expr#6=[>($t1, $t5)], DEPTNO=[$t0], G=[$t4], C=[$t2], $condition=[$t6])\n"
+ + " EnumerableUnion(all=[true])\n"
+ + " EnumerableCalc(expr#0..1=[{inputs}], expr#2=[0:BIGINT], DEPTNO=[$t0], $f1=[$t2], C=[$t1])\n"
+ + " EnumerableAggregate(group=[{7}], groups=[[{7}, {}]], C=[COUNT()])\n"
Review comment:
Why `{}` is in the groups?
Should it be `groups=[[{7}}]]`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366212096
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
Note: the current comment says *what* the code is doing.
However, it would be better to clarify *why* it is doing like that.
The map itself has no value here. The (business) value comes from splitting the grouping sets into the least number of `union all`. This intention is not really represented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366246964
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -110,8 +111,11 @@ private Resolved resolve() {
final Set<ImmutableBitSet> flatGroupSets =
Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final Map<ImmutableBitSet, Integer> groupSetCount = new HashMap<>();
for (List<ImmutableBitSet> groupSet : Linq4j.product(builder.build())) {
- flatGroupSets.add(ImmutableBitSet.union(groupSet));
+ final ImmutableBitSet set = ImmutableBitSet.union(groupSet);
+ groupSetCount.put(set, groupSetCount.getOrDefault(set, 0) + 1);
+ flatGroupSets.add(set);
Review comment:
I agree. We can unify HashMap and the sorted Set as TreeMap.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366174811
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
What does `(a, a, b, c, c, c, c)` mean here? How is it connected with `(a,b,c)` and `(a,c)`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366407494
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
For the entries in map, we do not care about whether they are sorted. But the flatten group sets (i.e.,the map's keyset) should be ordered when constructing Resolved.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1756: [CALCITE-1824] GROUP_ID
returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#issuecomment-575567843
Thanks, LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366178638
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
+ }
+ }
+ final RelNode project = relBuilder.project(selectList)
+ .rename(selectListNames).build();
+ projects.add(project);
+ }
+ if (projects.size() == 1) {
+ return projects.get(0);
Review comment:
Please add a brief comment on why is this present.
It is crystal clear that the code is returning the first item, however, it takes time to understand *why* is this needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366202853
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
They are duplicate group set before transformation into to canonical form `grouping sets (a, b, c)`. The `UnionAll`'s children and corresponding `group_id` is:
```
Union All
grouping sets(a, b, c) --- 0
grouping sets(a, c) --- 1
grouping sets(c) --- 2
grouping sets(c) --- 3
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366263800
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
Isn't a regular `Map` sufficient here by the way?
It does look weird that a `SortedMap` is passed to `Map` constructor, so the order of the entries is lost.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366212096
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
Note: the current comment says *what* the code is doing.
However, it would be better to clarify *why* it is doing like that.
The map itself has no value here. The value comes from splitting the grouping sets into the least number of `union all`. This intention is not really represented.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366246378
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
Thanks a lot. I update the comment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366252399
##########
File path: core/src/main/java/org/apache/calcite/sql/validate/AggregatingSelectScope.java
##########
@@ -108,19 +108,20 @@ private Resolved resolve() {
groupExprProjection = groupAnalyzer.groupExprProjection;
}
- final Set<ImmutableBitSet> flatGroupSets =
- Sets.newTreeSet(ImmutableBitSet.COMPARATOR);
+ final TreeMap<ImmutableBitSet, Integer> flatGroupSetCount =
Review comment:
Ok, it seems `SortedMap`'s name is more intuitive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366204707
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -4488,6 +4488,37 @@ private void startOfGroupStep3(String startOfGroup) {
"empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>. */
+ @Test public void testGroupId() {
+ CalciteAssert.that()
+ .with(CalciteAssert.Config.SCOTT)
+ .query("select deptno, group_id() + 1 as g, count(*) as c\n"
+ + "from \"scott\".emp\n"
+ + "group by grouping sets (deptno, deptno, deptno, (), ())\n"
+ + "having group_id() > 0")
+ .explainContains("EnumerableCalc(expr#0..2=[{inputs}], expr#3=[1], expr#4=[+($t1, $t3)], "
+ + "expr#5=[0], expr#6=[>($t1, $t5)], DEPTNO=[$t0], G=[$t4], C=[$t2], $condition=[$t6])\n"
+ + " EnumerableUnion(all=[true])\n"
+ + " EnumerableCalc(expr#0..1=[{inputs}], expr#2=[0:BIGINT], DEPTNO=[$t0], $f1=[$t2], C=[$t1])\n"
+ + " EnumerableAggregate(group=[{7}], groups=[[{7}, {}]], C=[COUNT()])\n"
Review comment:
Because the query is `grouping sets (deptno, deptno, deptno, (), ())`.
Therefore, both `{7}` abd `{} `can produce `group_id==1`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366176295
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
+ }
+ }
+ final RelNode project = relBuilder.project(selectList)
+ .rename(selectListNames).build();
+ projects.add(project);
+ }
+ if (projects.size() == 1) {
+ return projects.get(0);
+ }
+ return LogicalUnion.create(projects, true);
+ }
+
+ /**
+ * Add a group set to the group sets of a specific group id
+ */
+ private void addGroupSet(int groupId, ImmutableBitSet groupSet,
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets) {
+ final Set<ImmutableBitSet> groupSets = groupIdToGroupSets.getOrDefault(
+ groupId, Sets.newTreeSet(ImmutableBitSet.COMPARATOR));
Review comment:
Please prefer `computeIfAbsent` to avoid creating a tree set on each invocation to `addGroupSet`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on issue #1756: [CALCITE-1824]
GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on issue #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#issuecomment-575566057
Friendly ping @vlsi, any further work I need to do for this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366246619
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -4488,6 +4488,37 @@ private void startOfGroupStep3(String startOfGroup) {
"empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n");
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>. */
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] michaelmior closed pull request #1756: [CALCITE-1824]
GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
michaelmior closed pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366176531
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
+ }
+ }
+ final RelNode project = relBuilder.project(selectList)
+ .rename(selectListNames).build();
+ projects.add(project);
+ }
+ if (projects.size() == 1) {
+ return projects.get(0);
+ }
+ return LogicalUnion.create(projects, true);
+ }
+
+ /**
+ * Add a group set to the group sets of a specific group id
+ */
+ private void addGroupSet(int groupId, ImmutableBitSet groupSet,
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets) {
+ final Set<ImmutableBitSet> groupSets = groupIdToGroupSets.getOrDefault(
+ groupId, Sets.newTreeSet(ImmutableBitSet.COMPARATOR));
Review comment:
I guess `addGroupSet` could be removed if `computeIfAbsent` was used.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366202853
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
Review comment:
They are duplicate group set before transformation into to canonical form `grouping sets (a, b, c)`. The `UnionAll`'s children and corresponding `group_id` is:
```
Union All
grouping sets(a, b, c) --- 0
grouping sets(a, c) --- 1
grouping sets(c) --- 2
grouping sets(c) --- 3
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366177681
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
Review comment:
Why do you use `aggregateFieldNames` here rather than `fieldNamesIfNoRewrite`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1756:
[CALCITE-1824] GROUP_ID returns wrong result
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1756: [CALCITE-1824] GROUP_ID returns wrong result
URL: https://github.com/apache/calcite/pull/1756#discussion_r366203883
##########
File path: core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
##########
@@ -3039,6 +3040,113 @@ protected final void createAggImpl(
}
}
+ /**
+ * The {@code GROUP_ID()} function is used to distinguish duplicate groups.
+ * However, as Aggregate normalizes group sets (i.e., sorting, redundancy removal),
+ * this information is lost in RelNode. Therefore, it is impossible to
+ * implement the function in runtime.
+ *
+ * To fill this gap, an aggregation query that contains {@code GROUP_ID()} function
+ * will generally be rewritten into UNION when converting to RelNode.
+ *
+ * Also see the discussion in JIRA
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1824">[CALCITE-1824]
+ * GROUP_ID returns wrong result</a>.
+ */
+ private RelNode rewriteAggregateWithGroupId(Blackboard bb,
+ AggregatingSelectScope.Resolved r, AggConverter converter) {
+ final List<AggregateCall> aggregateCalls = converter.getAggCalls();
+ final ImmutableBitSet groupSet = r.groupSet;
+ final Map<ImmutableBitSet, Integer> groupSetCount = r.groupSetCount;
+
+ final List<String> fieldNamesIfNoRewrite = createAggregate(bb, groupSet,
+ r.groupSets, aggregateCalls).getRowType().getFieldNames();
+
+ // For every GROUP_ID value, collect its group sets in map
+ // E.g., GROUPING SETS (a, a, b, c, c, c, c), the map will be
+ // {0 -> (a, b, c), 1 -> (a, c), 2 -> (c), 3 -> (c)},
+ // in which the max GROUP_ID() value is 3
+ final Map<Integer, Set<ImmutableBitSet>> groupIdToGroupSets = new HashMap<>();
+ int maxGroupId = 0;
+ for (Map.Entry<ImmutableBitSet, Integer> entry: groupSetCount.entrySet()) {
+ int groupId = entry.getValue() - 1;
+ if (groupId > maxGroupId) {
+ maxGroupId = groupId;
+ }
+ for (int i = 0; i <= groupId; i++) {
+ addGroupSet(i, entry.getKey(), groupIdToGroupSets);
+ }
+ }
+
+ // AggregateCalls without GROUP_ID
+ final List<AggregateCall> aggregateCallsWithoutGroupId = new ArrayList<>();
+ for (AggregateCall aggregateCall : aggregateCalls) {
+ if (aggregateCall.getAggregation().kind != SqlKind.GROUP_ID) {
+ aggregateCallsWithoutGroupId.add(aggregateCall);
+ }
+ }
+
+ final List<RelNode> projects = new ArrayList<>();
+ // For each group id, we first construct an Aggregate without GROUP_ID()
+ // function call, and then create a Project node on top of it.
+ // The Project adds literal value for group id in right position.
+ for (int groupId = 0; groupId <= maxGroupId; groupId++) {
+ // Create the Aggregate node without GROUP_ID() call
+ final ImmutableList<ImmutableBitSet> groupSets =
+ ImmutableList.copyOf(groupIdToGroupSets.get(groupId));
+ final RelNode aggregate = createAggregate(bb, groupSet,
+ groupSets, aggregateCallsWithoutGroupId);
+
+ // RexLiteral for each GROUP_ID, note the type should be BIGINT
+ final RelDataType groupIdType = typeFactory.createSqlType(SqlTypeName.BIGINT);
+ final RexNode groupIdLiteral = rexBuilder.makeExactLiteral(
+ BigDecimal.valueOf(groupId), groupIdType);
+
+ relBuilder.push(aggregate);
+ final List<String> aggregateFieldNames = aggregate.getRowType().getFieldNames();
+
+ final List<RexNode> selectList = new ArrayList<>();
+ final List<String> selectListNames = new ArrayList<>();
+ final int groupExprLength = r.groupExprList.size();
+ // Project fields from group by expressions
+ for (int i = 0; i < groupExprLength; i++) {
+ selectList.add(relBuilder.field(i));
+ selectListNames.add(aggregateFieldNames.get(i));
+ }
+ // Project fields from aggregate calls
+ int groupIdCount = 0;
+ for (int i = 0; i < aggregateCalls.size(); i++) {
+ if (aggregateCalls.get(i).getAggregation().kind == SqlKind.GROUP_ID) {
+ selectList.add(groupIdLiteral);
+ selectListNames.add(fieldNamesIfNoRewrite.get(groupExprLength + i));
+ groupIdCount++;
+ } else {
+ int ordinal = groupExprLength + i - groupIdCount;
+ selectList.add(relBuilder.field(ordinal));
+ selectListNames.add(aggregateFieldNames.get(ordinal));
Review comment:
My initial intention is to do some type validation. But it seems to be unnecessary. We can remove `aggregateFieldNames `and use `fieldNamesIfNoRewrite` directly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services