New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom Calcite Rule to remove redundant references #16402
Conversation
.../java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java
Fixed
Show fixed
Hide fixed
.../java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java
Fixed
Show fixed
Hide fixed
sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateProjectMergeRule.java
Fixed
Show fixed
Hide fixed
.../java/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateProjectMergeRule.java
Fixed
Show fixed
Hide fixed
...a/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateRemoveRedundancyRule.java
Fixed
Show fixed
Hide fixed
...a/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateRemoveRedundancyRule.java
Fixed
Show fixed
Hide fixed
.../main/java/org/apache/druid/sql/calcite/rule/logical/DruidAggregateRemoveRedundancyRule.java
Fixed
Show fixed
Hide fixed
...a/org/apache/druid/sql/calcite/rule/logical/DruidImmutableAggregateRemoveRedundancyRule.java
Fixed
Show fixed
Hide fixed
AggregatorFactory factory = new GroupingAggregatorFactory(name, arguments); | ||
AggregatorFactory factory = new GroupingAggregatorFactory( | ||
name, | ||
arguments.stream().distinct().collect(Collectors.toList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this will mask the newly added check in GroupingAggregatorFactory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made this a soft exception and removed this distinct masking here
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
*/ | ||
@Value.Enclosing | ||
public class DruidAggregateRemoveRedundancyRule | ||
extends RelRule<DruidAggregateRemoveRedundancyRule.Config> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made use of RelOptRule and got rid of the Immutable class
|
||
private DruidAggregateRemoveRedundancyRule() | ||
{ | ||
super(operand(Aggregate.class, operand(Project.class, any()))); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
RelOptRule.operand
|
||
private DruidAggregateRemoveRedundancyRule() | ||
{ | ||
super(operand(Aggregate.class, operand(Project.class, any()))); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
RelOptRule.operand
|
||
private DruidAggregateRemoveRedundancyRule() | ||
{ | ||
super(operand(Aggregate.class, operand(Project.class, any()))); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
RelOptRule.any
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
try { | ||
factory = new GroupingAggregatorFactory(name, arguments); | ||
} | ||
catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 factoryCannotBuildQueryException
only takes in the message of the exception, which would make things harder to debug without a stack if there's an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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: