mihaibudiu commented on code in PR #4495:
URL: https://github.com/apache/calcite/pull/4495#discussion_r2263698591
##########
core/src/test/resources/sql/agg.iq:
##########
@@ -3897,4 +3897,243 @@ EnumerableCalc(expr#0=[{inputs}],
expr#1=[CAST($t0):BIGINT NOT NULL], M=[$t1])
EnumerableTableScan(table=[[scott, EMP]])
!plan
+# [CALCITE-7126] The calculation result of grouping function is wrong
+SELECT deptno, job, sal, SUM(comm),
+ GROUPING(deptno) AS deptno_flag,
+ GROUPING(job) AS job_flag,
+ GROUPING(sal) AS sal_flag
+FROM emp
+GROUP BY GROUPING SETS ((deptno, job), (deptno, sal), (deptno, job))
+ORDER BY deptno, job, sal;
++--------+-----------+---------+---------+-------------+----------+----------+
+| DEPTNO | JOB | SAL | EXPR$3 | DEPTNO_FLAG | JOB_FLAG | SAL_FLAG |
++--------+-----------+---------+---------+-------------+----------+----------+
+| 10 | CLERK | | | 0 | 0 | 1 |
+| 10 | CLERK | | | 0 | 0 | 1 |
+| 10 | MANAGER | | | 0 | 0 | 1 |
+| 10 | MANAGER | | | 0 | 0 | 1 |
+| 10 | PRESIDENT | | | 0 | 0 | 1 |
+| 10 | PRESIDENT | | | 0 | 0 | 1 |
+| 10 | | 1300.00 | | 0 | 1 | 0 |
+| 10 | | 2450.00 | | 0 | 1 | 0 |
+| 10 | | 5000.00 | | 0 | 1 | 0 |
+| 20 | ANALYST | | | 0 | 0 | 1 |
+| 20 | ANALYST | | | 0 | 0 | 1 |
+| 20 | CLERK | | | 0 | 0 | 1 |
+| 20 | CLERK | | | 0 | 0 | 1 |
+| 20 | MANAGER | | | 0 | 0 | 1 |
+| 20 | MANAGER | | | 0 | 0 | 1 |
+| 20 | | 800.00 | | 0 | 1 | 0 |
+| 20 | | 1100.00 | | 0 | 1 | 0 |
+| 20 | | 2975.00 | | 0 | 1 | 0 |
+| 20 | | 3000.00 | | 0 | 1 | 0 |
+| 30 | CLERK | | | 0 | 0 | 1 |
+| 30 | CLERK | | | 0 | 0 | 1 |
+| 30 | MANAGER | | | 0 | 0 | 1 |
+| 30 | MANAGER | | | 0 | 0 | 1 |
+| 30 | SALESMAN | | 2200.00 | 0 | 0 | 1 |
+| 30 | SALESMAN | | 2200.00 | 0 | 0 | 1 |
+| 30 | | 950.00 | | 0 | 1 | 0 |
+| 30 | | 1250.00 | 1900.00 | 0 | 1 | 0 |
+| 30 | | 1500.00 | 0.00 | 0 | 1 | 0 |
+| 30 | | 1600.00 | 300.00 | 0 | 1 | 0 |
+| 30 | | 2850.00 | | 0 | 1 | 0 |
++--------+-----------+---------+---------+-------------+----------+----------+
+(30 rows)
+
+!ok
+
+EnumerableCalc(expr#0..5=[{inputs}], expr#6=[0:BIGINT], proj#0..3=[{exprs}],
DEPTNO_FLAG=[$t6], JOB_FLAG=[$t4], SAL_FLAG=[$t5])
+ EnumerableMergeUnion(all=[true])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC],
dir2=[ASC])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[null:DECIMAL(7, 2)],
expr#4=[0:BIGINT], expr#5=[1:BIGINT], DEPTNO=[$t1], JOB=[$t0], SAL=[$t3],
EXPR$3=[$t2], JOB_FLAG=[$t4], SAL_FLAG=[$t5])
+ EnumerableAggregate(group=[{2, 7}], EXPR$3=[SUM($6)])
+ EnumerableTableScan(table=[[scott, EMP]])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC],
dir2=[ASC])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[null:DECIMAL(7, 2)],
expr#4=[0:BIGINT], expr#5=[1:BIGINT], DEPTNO=[$t1], JOB=[$t0], SAL=[$t3],
EXPR$3=[$t2], JOB_FLAG=[$t4], SAL_FLAG=[$t5])
+ EnumerableAggregate(group=[{2, 7}], EXPR$3=[SUM($6)])
+ EnumerableTableScan(table=[[scott, EMP]])
+ EnumerableSort(sort0=[$0], sort1=[$1], sort2=[$2], dir0=[ASC], dir1=[ASC],
dir2=[ASC])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[null:VARCHAR(9)],
expr#4=[1:BIGINT], expr#5=[0:BIGINT], DEPTNO=[$t1], JOB=[$t3], SAL=[$t0],
EXPR$3=[$t2], JOB_FLAG=[$t4], SAL_FLAG=[$t5])
+ EnumerableAggregate(group=[{5, 7}], EXPR$3=[SUM($6)])
+ EnumerableTableScan(table=[[scott, EMP]])
+!plan
+
+# Using both GROUP_ID() and GROUPING functions in SQL aggregation queries with
duplicate groupings in GROUPING SETS
+SELECT deptno, job, sal, SUM(comm),
+ GROUPING(deptno, sal) AS deptno_sal_flag,
+ GROUP_ID() AS g
+FROM emp
+GROUP BY GROUPING SETS ((deptno, job), (deptno, sal), (deptno, job), (),
(sal), (sal))
+ORDER BY deptno, job, sal;
++--------+-----------+---------+---------+-----------------+---+
+| DEPTNO | JOB | SAL | EXPR$3 | DEPTNO_SAL_FLAG | G |
++--------+-----------+---------+---------+-----------------+---+
+| 10 | CLERK | | | 1 | 0 |
+| 10 | CLERK | | | 1 | 1 |
+| 10 | MANAGER | | | 1 | 0 |
+| 10 | MANAGER | | | 1 | 1 |
+| 10 | PRESIDENT | | | 1 | 0 |
+| 10 | PRESIDENT | | | 1 | 1 |
+| 10 | | 1300.00 | | 0 | 0 |
+| 10 | | 2450.00 | | 0 | 0 |
+| 10 | | 5000.00 | | 0 | 0 |
+| 20 | ANALYST | | | 1 | 0 |
+| 20 | ANALYST | | | 1 | 1 |
+| 20 | CLERK | | | 1 | 0 |
+| 20 | CLERK | | | 1 | 1 |
+| 20 | MANAGER | | | 1 | 0 |
+| 20 | MANAGER | | | 1 | 1 |
+| 20 | | 800.00 | | 0 | 0 |
+| 20 | | 1100.00 | | 0 | 0 |
+| 20 | | 2975.00 | | 0 | 0 |
+| 20 | | 3000.00 | | 0 | 0 |
+| 30 | CLERK | | | 1 | 0 |
+| 30 | CLERK | | | 1 | 1 |
+| 30 | MANAGER | | | 1 | 0 |
+| 30 | MANAGER | | | 1 | 1 |
+| 30 | SALESMAN | | 2200.00 | 1 | 0 |
+| 30 | SALESMAN | | 2200.00 | 1 | 1 |
+| 30 | | 950.00 | | 0 | 0 |
+| 30 | | 1250.00 | 1900.00 | 0 | 0 |
+| 30 | | 1500.00 | 0.00 | 0 | 0 |
+| 30 | | 1600.00 | 300.00 | 0 | 0 |
+| 30 | | 2850.00 | | 0 | 0 |
+| | | 800.00 | | 2 | 0 |
+| | | 800.00 | | 2 | 1 |
+| | | 950.00 | | 2 | 0 |
+| | | 950.00 | | 2 | 1 |
+| | | 1100.00 | | 2 | 0 |
+| | | 1100.00 | | 2 | 1 |
+| | | 1250.00 | 1900.00 | 2 | 0 |
+| | | 1250.00 | 1900.00 | 2 | 1 |
+| | | 1300.00 | | 2 | 0 |
+| | | 1300.00 | | 2 | 1 |
+| | | 1500.00 | 0.00 | 2 | 0 |
+| | | 1500.00 | 0.00 | 2 | 1 |
+| | | 1600.00 | 300.00 | 2 | 0 |
+| | | 1600.00 | 300.00 | 2 | 1 |
+| | | 2450.00 | | 2 | 0 |
+| | | 2450.00 | | 2 | 1 |
+| | | 2850.00 | | 2 | 0 |
+| | | 2850.00 | | 2 | 1 |
+| | | 2975.00 | | 2 | 0 |
+| | | 2975.00 | | 2 | 1 |
+| | | 3000.00 | | 2 | 0 |
+| | | 3000.00 | | 2 | 1 |
+| | | 5000.00 | | 2 | 0 |
+| | | 5000.00 | | 2 | 1 |
+| | | | 2200.00 | 3 | 0 |
++--------+-----------+---------+---------+-----------------+---+
+(55 rows)
+
+!ok
+
+EnumerableMergeUnion(all=[true])
Review Comment:
I find that the high-level plans `!explain-validated-on` more useful.
Is there any particular reason you prefer the enumerable plans?
##########
core/src/main/java/org/apache/calcite/sql/SqlOperatorBinding.java:
##########
@@ -75,6 +75,10 @@ public int getGroupCount() {
return -1;
}
+ public boolean allowChangeNullable() {
Review Comment:
Please add a JavaDoc here. This is very obscure.
##########
core/src/main/java/org/apache/calcite/rel/core/AggregateCall.java:
##########
@@ -122,7 +123,8 @@ private AggregateCall(SqlParserPos pos, SqlAggFunction
aggFunction, boolean dist
boolean approximate, boolean ignoreNulls,
List<RexNode> rexList, List<Integer> argList,
int filterArg, @Nullable ImmutableBitSet distinctKeys,
- RelCollation collation, RelDataType type, @Nullable String name) {
+ RelCollation collation, RelDataType type, @Nullable String name,
+ boolean allowChangeNullable) {
Review Comment:
The new field needs to be documented.
If you try to write documentation for it you will realize that something is
off.
Maybe if the field was called "resultMayBeNullable" it would make more sense.
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -2819,28 +2819,56 @@ private RelBuilder aggregate_(ImmutableBitSet groupSet,
* GROUP_ID returns wrong result</a> and
* <a
href="https://issues.apache.org/jira/browse/CALCITE-4748";>[CALCITE-4748]
* If there are duplicate GROUPING SETS, Calcite should return duplicate
- * rows</a>.
+ * rows</a> and
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7126";>[CALCITE-7126]
+ * The calculation result of grouping function is wrong</a>.
*/
private RelBuilder rewriteAggregateWithDuplicateGroupSets(
ImmutableBitSet groupSet,
ImmutableSortedMultiset<ImmutableBitSet> groupSets,
List<AggCallPlus> aggregateCalls) {
+ List<AggregateCall> calls =
+ aggregateCalls.stream()
+ .map(AggCallPlus::aggregateCall)
+ .collect(Collectors.toList());
+ return rewriteAggregateWithDuplicateGroupSetsImpl(groupSet, groupSets,
calls);
+ }
+
+ private RelBuilder rewriteAggregateWithDuplicateGroupSetsImpl(
Review Comment:
this could probably be called "rewriteGroupingSets"
##########
core/src/main/java/org/apache/calcite/rel/core/Aggregate.java:
##########
@@ -612,7 +616,19 @@ public AggCallBinding(RelDataTypeFactory typeFactory,
SqlAggFunction aggFunction, List<RelDataType> operands, int groupCount,
boolean filter) {
this(typeFactory, aggFunction, ImmutableList.of(), operands, groupCount,
- filter);
+ filter, true);
+ }
+
+ @Deprecated
Review Comment:
can you also add a comment suggesting the alternative API?
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -4446,8 +4531,12 @@ private class AggCallImpl implements AggCallPlus {
// return a call that is "approximately equivalent ... and is good for
// deriving field names", so dummy values are good enough.
final RelCollation collation = RelCollations.EMPTY;
- final RelDataType type =
- getTypeFactory().createSqlType(SqlTypeName.BOOLEAN);
+ final RelDataType type;
+ if (aggFunction.getKind() == SqlKind.GROUP_ID) {
+ type = getTypeFactory().createSqlType(SqlTypeName.BIGINT);
Review Comment:
Why not just INT?
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -2849,55 +2877,112 @@ private RelBuilder
rewriteAggregateWithDuplicateGroupSets(
maxGroupId = groupId;
}
for (int i = 0; i <= groupId; i++) {
- groupIdToGroupSets.computeIfAbsent(i,
- k -> Sets.newTreeSet(ImmutableBitSet.COMPARATOR))
+ groupIdToGroupSets.computeIfAbsent(i, k ->
Sets.newTreeSet(ImmutableBitSet.COMPARATOR))
Review Comment:
there seems to be no change here
--
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]
