You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2024/05/09 08:40:14 UTC
Re: [PR] Custom Calcite Rule to remove redundant references (druid)
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org