kgyrtkirk commented on code in PR #16402:
URL: https://github.com/apache/druid/pull/16402#discussion_r1595126631
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/GroupingSqlAggregator.java:
##########
@@ -90,7 +90,10 @@ public Aggregation toDruidAggregation(
}
}
}
- AggregatorFactory factory = new GroupingAggregatorFactory(name, arguments);
+ AggregatorFactory factory = new GroupingAggregatorFactory(
+ name,
+ arguments.stream().distinct().collect(Collectors.toList())
Review Comment:
wouldn't this will mask the newly added check in `GroupingAggregatorFactory`
?
##########
sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/aggregateRemoveRedundancy.iq:
##########
@@ -0,0 +1,177 @@
+!set useApproximateCountDistinct false
+!set useGroupingSetForExactDistinct true
+!use druidtest://?numMergeBuffers=3
+!set outputformat mysql
+
+SELECT
+ TIME_FLOOR(__time, 'PT1H') as t,
+ COUNT(DISTINCT CASE WHEN channel = '#it.wikipedia' THEN user END) as
cnt_case,
+ COUNT(DISTINCT user) FILTER (WHERE channel = '#it.wikipedia') as cnt_filter,
+ COUNT(DISTINCT user) as cnt
+FROM "wikipedia"
+GROUP BY 1;
++---------------------+----------+------------+-----+
+| t | cnt_case | cnt_filter | cnt |
++---------------------+----------+------------+-----+
+| 2015-09-12 00:00:00 | 5 | 5 | 149 |
+| 2015-09-12 01:00:00 | 14 | 14 | 506 |
+| 2015-09-12 02:00:00 | 10 | 10 | 459 |
+| 2015-09-12 03:00:00 | 10 | 10 | 427 |
+| 2015-09-12 04:00:00 | 6 | 6 | 427 |
+| 2015-09-12 05:00:00 | 10 | 10 | 448 |
+| 2015-09-12 06:00:00 | 13 | 13 | 498 |
+| 2015-09-12 07:00:00 | 21 | 21 | 574 |
+| 2015-09-12 08:00:00 | 36 | 36 | 707 |
+| 2015-09-12 09:00:00 | 44 | 44 | 770 |
+| 2015-09-12 10:00:00 | 37 | 37 | 785 |
+| 2015-09-12 11:00:00 | 40 | 40 | 799 |
+| 2015-09-12 12:00:00 | 45 | 45 | 855 |
+| 2015-09-12 13:00:00 | 44 | 44 | 905 |
+| 2015-09-12 14:00:00 | 48 | 48 | 886 |
+| 2015-09-12 15:00:00 | 37 | 37 | 949 |
+| 2015-09-12 16:00:00 | 50 | 50 | 969 |
+| 2015-09-12 17:00:00 | 38 | 38 | 941 |
+| 2015-09-12 18:00:00 | 40 | 40 | 925 |
+| 2015-09-12 19:00:00 | 32 | 32 | 930 |
+| 2015-09-12 20:00:00 | 31 | 31 | 882 |
+| 2015-09-12 21:00:00 | 40 | 40 | 861 |
+| 2015-09-12 22:00:00 | 28 | 28 | 716 |
+| 2015-09-12 23:00:00 | 20 | 20 | 631 |
++---------------------+----------+------------+-----+
+(24 rows)
+
+!ok
+
+LogicalAggregate(group=[{0}], cnt_case=[COUNT(DISTINCT $1)],
cnt_filter=[COUNT(DISTINCT $2) FILTER $3], cnt=[COUNT(DISTINCT $2)])
+ LogicalProject(t=[TIME_FLOOR($0, 'PT1H')], $f1=[CASE(=($1, '#it.wikipedia'),
$16, null:VARCHAR)], user=[$16], $f3=[IS TRUE(=($1, '#it.wikipedia'))])
+ LogicalTableScan(table=[[druid, wikipedia]])
+
+!convertedPlan
+
+LogicalAggregate(group=[{0}], cnt_case=[COUNT(DISTINCT $1)],
cnt_filter=[COUNT(DISTINCT $2) FILTER $3], cnt=[COUNT(DISTINCT $2)])
+ LogicalProject(t=[TIME_FLOOR($0, 'PT1H')], $f1=[CASE(=($1, '#it.wikipedia'),
$16, null:VARCHAR)], user=[$16], $f3=[IS TRUE(=($1, '#it.wikipedia'))])
+ LogicalTableScan(table=[[druid, wikipedia]])
+
+!logicalPlan
Review Comment:
its very sad to see that we can't capture the last valid logical plan from
which the resulting query was generated...unfortunately we can't do much as
this is how the old planner works...
if we would be able to show that - then we could just remove the native plan
which is much harder to read
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -15689,4 +15688,56 @@ public void testStringOperationsNullableInference()
)
);
}
+
+ @SqlTestFrameworkConfig(numMergeBuffers = 4)
+ @Test
+ public void testGroupingSetsWithAggrgateCase()
Review Comment:
I'm not sure what we gain from having the same testcase here and in an `iq`
file as well
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
