xiedeyantu commented on code in PR #4499:
URL: https://github.com/apache/calcite/pull/4499#discussion_r2285251408
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -3118,12 +3118,11 @@ LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0},
{}]], EXPR$2=[COUNT(DISTI
</Resource>
<Resource name="planAfter">
<![CDATA[
-LogicalProject(DEPTNO=[$0], JOB=[$1], EXPR$2=[$2], EXPR$3=[CAST($3):INTEGER
NOT NULL])
- LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0}, {}]],
EXPR$2=[COUNT($2) FILTER $4], EXPR$3=[MIN($3) FILTER $5])
- LogicalProject(DEPTNO=[$0], JOB=[$1], ENAME=[$2], EXPR$3=[$3], $g_0=[=($4,
0)], $g_1=[=($4, 1)])
- LogicalAggregate(group=[{0, 1, 2}], groups=[[{0, 1, 2}, {0, 1}]],
EXPR$3=[SUM($3)], $g=[GROUPING($0, $1, $2)])
- LogicalProject(DEPTNO=[$7], JOB=[$2], ENAME=[$1], SAL=[$5])
- LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+LogicalAggregate(group=[{0, 1}], groups=[[{0, 1}, {0}, {}]], EXPR$2=[COUNT($2)
FILTER $4], EXPR$3=[MIN($3) FILTER $5])
Review Comment:
This plan looks comfortable.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -754,9 +755,16 @@ protected RexNode removeCorrelationExpr(
: requireNonNull(combinedMap.get(oldAggCall.filterArg),
() -> "combinedMap.get(" + oldAggCall.filterArg + ")");
+ boolean newHasEmptyGroup = newGroupSets == null && newGroupSet.isEmpty();
+ if (newGroupSets != null) {
Review Comment:
Can the `boolean hasEmptyGroup(List<SqlNode> groupClause)` method be reused
here?
##########
core/src/main/java/org/apache/calcite/rel/core/Aggregate.java:
##########
@@ -578,8 +582,9 @@ public static List<Integer> getRollup(List<ImmutableBitSet>
groupSets) {
public static class AggCallBinding extends SqlOperatorBinding {
private final List<RelDataType> preOperands;
private final List<RelDataType> operands;
- private final int groupCount;
+ private int groupCount;
Review Comment:
final?
##########
core/src/main/java/org/apache/calcite/rel/core/AggregateCall.java:
##########
@@ -573,6 +618,7 @@ public AggregateCall copy(List<Integer> argList) {
* @param newGroupKeyCount number of columns in GROUP BY of new aggregate
* @return AggregateCall that suits new inputs and GROUP BY columns
*/
+ @Deprecated
Review Comment:
@Deprecated // to be removed before 2.0
##########
core/src/main/java/org/apache/calcite/rel/rules/AggregateUnionTransposeRule.java:
##########
@@ -117,17 +116,12 @@ public AggregateUnionTransposeRule(Class<? extends
Aggregate> aggregateClass,
return;
}
- int groupCount = aggRel.getGroupSet().cardinality();
-
- List<AggregateCall> transformedAggCalls =
- transformAggCalls(
- aggRel.copy(aggRel.getTraitSet(), aggRel.getInput(),
- aggRel.getGroupSet(), null, aggRel.getAggCallList()),
- groupCount, aggRel.getAggCallList());
- if (transformedAggCalls == null) {
- // we've detected the presence of something like AVG,
- // which we can't handle
- return;
+ for (AggregateCall aggCall : aggRel.getAggCallList()) {
Review Comment:
This looks OK, it is better to change this in another pr next time.
##########
core/src/main/java/org/apache/calcite/rel/core/Aggregate.java:
##########
@@ -615,10 +648,15 @@ public AggCallBinding(RelDataTypeFactory typeFactory,
filter);
}
+ @Deprecated
Review Comment:
@Deprecated // to be removed before 2.0
##########
core/src/main/java/org/apache/calcite/rel/core/Aggregate.java:
##########
@@ -590,7 +595,11 @@ public static class AggCallBinding extends
SqlOperatorBinding {
* @param operands Data types of operands
* @param groupCount Number of columns in the GROUP BY clause
* @param filter Whether the aggregate function has a FILTER clause
+ *
+ * @deprecated Use
+ * {@link #AggCallBinding(RelDataTypeFactory, SqlAggFunction, List, List,
boolean, boolean)}
*/
+ @Deprecated
Review Comment:
@Deprecated // to be removed before 2.0
##########
spark/src/test/java/org/apache/calcite/test/SparkAdapterTest.java:
##########
@@ -144,8 +144,8 @@ private CalciteAssert.AssertQuery sql(String sql) {
final String plan = "PLAN="
+ "EnumerableCalc(expr#0..4=[{inputs}], expr#5=[CAST($t3):BIGINT NOT
NULL], proj#0..2=[{exprs}], CNT_Y=[$t5], CNT_DIST_Y=[$t4])\n"
+ " EnumerableAggregate(group=[{}], SUM_X=[MIN($1) FILTER $6],
MIN_Y=[MIN($2) FILTER $6], MAX_Y=[MIN($3) FILTER $6], CNT_Y=[MIN($4) FILTER
$6], CNT_DIST_Y=[COUNT($0) FILTER $5])\n"
- + " EnumerableCalc(expr#0..5=[{inputs}], expr#6=[0], expr#7=[=($t5,
$t6)], expr#8=[1], expr#9=[=($t5, $t8)], proj#0..4=[{exprs}], $g_0=[$t7],
$g_1=[$t9])\n"
- + " EnumerableAggregate(group=[{1}], groups=[[{1}, {}]],
SUM_X=[$SUM0($0)], MIN_Y=[MIN($1)], MAX_Y=[MAX($1)], CNT_Y=[COUNT()],
$g=[GROUPING($1)])\n"
+ + " EnumerableCalc(expr#0..5=[{inputs}], expr#6=[0], expr#7=[=($t2,
$t6)], expr#8=[null:INTEGER], expr#9=[CASE($t7, $t8, $t1)], expr#10=[=($t5,
$t6)], expr#11=[1], expr#12=[=($t5, $t11)], Y=[$t0], SUM_X=[$t9], MIN_Y=[$t3],
MAX_Y=[$t4], CNT_Y=[$t2], $g_0=[$t10], $g_1=[$t12])\n"
Review Comment:
Is it just a position shift of AggCall?
--
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]
