Custom Calcite Rule to remove redundant references #16402

sreemanamala · 2024-05-06T15:52:23Z

Description

Custom calcite rule mimicking AggregateProjectMergeRule to extend support to expressions.
The current calcite rule return null in such cases.
In addition, this removes the redundant references.

Key changed/added classes in this PR

DruidAggregateRemoveRedundancyRule
CalciteRulesManager

This PR has:

.../java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java

sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateProjectMergeRule.java

.../java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java

...a/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateRemoveRedundancyRule.java

.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java

...a/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateRemoveRedundancyRule.java

kgyrtkirk · 2024-05-09T08:16:46Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/GroupingSqlAggregator.java

- AggregatorFactory factory = new GroupingAggregatorFactory(name, arguments);
+ AggregatorFactory factory = new GroupingAggregatorFactory(
+ name,
+ arguments.stream().distinct().collect(Collectors.toList())


wouldn't this will mask the newly added check in GroupingAggregatorFactory ?

made this a soft exception and removed this distinct masking here

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

kgyrtkirk · 2024-05-09T08:38:32Z

sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/aggregateRemoveRedundancy.iq

+ 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


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

kgyrtkirk · 2024-05-09T09:18:58Z

.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java

+ */
+@Value.Enclosing
+public class DruidAggregateRemoveRedundancyRule
+ extends RelRule<DruidAggregateRemoveRedundancyRule.Config>


you could possibly extend RelOptRule instead; that doesn't need this config stuff;

or optionally move that immutables generated stuff inside this class somewhere - so its co-located

made use of RelOptRule and got rid of the Immutable class

.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java

+
+ private DruidAggregateRemoveRedundancyRule()
+ {
+ super(operand(Aggregate.class, operand(Project.class, any())));


.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java

+
+ private DruidAggregateRemoveRedundancyRule()
+ {
+ super(operand(Aggregate.class, operand(Project.class, any())));


.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java

+
+ private DruidAggregateRemoveRedundancyRule()
+ {
+ super(operand(Aggregate.class, operand(Project.class, any())));


kgyrtkirk · 2024-05-09T12:41:13Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

@@ -12941,8 +12941,7 @@ public void testRepeatedIdenticalVirtualExpressionGrouping()
 .setVirtualColumns(expressionVirtualColumn("v0", "1", ColumnType.LONG))
 .setDimensions(
 dimensions(
- new DefaultDimensionSpec("v0", "d0", ColumnType.LONG),
- new DefaultDimensionSpec("v0", "d1", ColumnType.LONG)
+ new DefaultDimensionSpec("v0", "d0", ColumnType.LONG)


I was looking at this for a while - but actually its ok; since the case is reduced to TRUE because dim1 = NULL may never be TRUE

rohangarg · 2024-05-10T12:13:36Z

sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/GroupingSqlAggregator.java

+ try {
+ factory = new GroupingAggregatorFactory(name, arguments);
+ }
+ catch (Exception e) {


pass by comment, is this the correct way to handle any errors coming out of the agg factory?
A couple of things that I find ambiguous :

Grouping Aggregation [%s] is not supported seems like misleading message incase there's an actual bug in the agg factory

CannotBuildQueryException only takes in the message of the exception, which would make things harder to debug without a stack if there's an exception.

@rohangarg Thanks for the comment.
I have updated the exception message to remove not supported. I think the exception here would only be caused by the Precondition checks and that could be easily understood by the exception message
Made the exception look similar to the one we have in EarliestLatestBySqlAggregator and return null.
anyways, In the end DruidQuery would throw CannotBuildQueryException. So it made sense to not throw in the aggregator class.

Does this look good to you now?

Thanks for the fixups :)
A couple of things I would check are :

How does the whole user message look when the exception is thrown - does that make sense?

Are all the exceptions thrown by the constructor distinguishable via the message - so that we aren't left with ambiguity when an exception arrives?

If both those things look fine, then it is good to go from my side too

These exceptions which would be thrown here would result in not being able to plan the query in the current path and look for other ways. The custom rule which is introduced in this PR will make sure that the duplicate grouping would be removed and plan the query properly. In my understanding, the user would face this exception if the query can not be planned at all. As part of this testcase which I have added, Initially this would fail in plan and then turns towards the rule, gets converted and generated the final plan.

By setting the planning error with the exception being received, this should be able to distinguish between the different exceptions based on the message

Ah, for 1, I was actually mentioning the meaningfulness of planning error message which would get surfaced if a query can't get planned. It does look reasonable to me.
For 2, thanks for the clarification.
LGTM

sreemanamala added 5 commits May 5, 2024 22:03

initial

63208f4

DruidAggregateProjectMergeRule

9d5561d

Merge branch 'master' into grouping-sets

76d0c47

fix other failures

f972b16

updated doc

824ab85

github-actions bot added the Area - Querying label May 6, 2024

github-advanced-security bot found potential problems May 6, 2024

View reviewed changes

sreemanamala added 2 commits May 6, 2024 22:08

distinct groupings in grouping aggregator

26fad89

fixing failures in case of decoupled mode

a649acc

github-advanced-security bot found potential problems May 7, 2024

View reviewed changes

tests

e82b4a9

sreemanamala changed the title ~~Custom Aggregate Project Merge Rule capable of merging expressions~~ Custom Calcite Rule to remove redundant references May 7, 2024

sreemanamala added 3 commits May 7, 2024 21:18

Trigger Build

b174754

refactor

4152d44

refactor

190d7ef

kgyrtkirk reviewed May 9, 2024

View reviewed changes

cryptoe modified the milestone: 30.0.0 May 9, 2024

sreemanamala added 5 commits May 9, 2024 21:28

review comments

0c47a08

refactor

500db4c

Trigger Build

73f52e5

Merge branch 'master' into grouping-sets

78e55a3

refactor

43f8744

github-advanced-security bot found potential problems May 10, 2024

View reviewed changes

old test fix

a244686

kgyrtkirk approved these changes May 10, 2024

View reviewed changes

rohangarg reviewed May 10, 2024

View reviewed changes

refactor

343f158

kgyrtkirk merged commit b8dd747 into apache:master May 14, 2024
87 checks passed

kgyrtkirk added the Bug label May 14, 2024

sreemanamala deleted the grouping-sets branch May 20, 2024 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Calcite Rule to remove redundant references #16402

Custom Calcite Rule to remove redundant references #16402

sreemanamala commented May 6, 2024 •

edited

kgyrtkirk May 9, 2024

sreemanamala May 9, 2024

kgyrtkirk May 9, 2024

kgyrtkirk May 9, 2024

sreemanamala May 9, 2024

kgyrtkirk May 9, 2024

rohangarg May 10, 2024

sreemanamala May 10, 2024

rohangarg May 13, 2024

sreemanamala May 13, 2024

rohangarg May 13, 2024

Custom Calcite Rule to remove redundant references #16402

Custom Calcite Rule to remove redundant references #16402

Conversation

sreemanamala commented May 6, 2024 • edited

Description

Key changed/added classes in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sreemanamala commented May 6, 2024 •

edited