xuzifu666 commented on code in PR #5064:
URL: https://github.com/apache/calcite/pull/5064#discussion_r3503748294
##########
core/src/main/java/org/apache/calcite/sql/dialect/PrestoSqlDialect.java:
##########
@@ -175,6 +175,13 @@ private static void unparseUsingLimit(SqlWriter writer,
@Nullable SqlNode offset
return true;
}
+ @Override public SqlNode rewriteGroupingIdExpr(SqlNode aggCall) {
Review Comment:
This is completely redundant with the PostgreSQL dialect; Maybe it can be
extracted into a common method.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1032,6 +1032,45 @@ private static String toSql(RelNode root, SqlDialect
dialect,
relFn(relFn).ok(expectedSql);
}
+ @Test void testGroupingIdRewriteInPostgresql() {
Review Comment:
Please add jira link to the test method.
Is it certain that these SQL statements will run successfully on PostgreSQL?
note: The three existing tests all use only the single-parameter
`GROUPING_ID(DEPTNO)`. Since the core scenario described in the task involves
the multi-parameter `GROUPING_ID(a, b)`, I suggest adding a multi-parameter
test case—which highlights the key difference where the legacy database's
`GROUPING` function supported only a single parameter—to verify the correctness
of the multi-parameter rewrite.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1283,10 +1283,14 @@ private void addOrderItem(List<SqlNode> orderByList,
/** Converts a call to an aggregate function to an expression. */
public SqlNode toSql(AggregateCall aggCall) {
- return toSql(aggCall.getAggregation(), aggCall.isDistinct(),
+ SqlNode node = toSql(aggCall.getAggregation(), aggCall.isDistinct(),
Util.transform(aggCall.rexList, e -> toSql(null, e)),
Util.transform(aggCall.getArgList(), this::field),
aggCall.filterArg, aggCall.collation, aggCall.isApproximate());
+ if (aggCall.getAggregation().getName().equals("GROUPING_ID")) {
Review Comment:
Hard code here, could `SqlStdOperatorTable.GROUPING_ID.getName()` be used
here to avoid the string becoming out of sync if the name changes in the future?
--
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]
