Feature configurable calcite bloat #16248
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java
Fixed
Show fixed
Hide fixed
| @@ -82,6 +83,10 @@ public class CalciteRulesManager | |||
| private static final int HEP_DEFAULT_MATCH_LIMIT = Integer.parseInt( | |||
| System.getProperty(HEP_DEFAULT_MATCH_LIMIT_CONFIG_STRING, "1200") | |||
| ); | |||
| private static final String BLOAT_PROPERTY = "druid.sql.planner.bloat"; | |||
| private static final int BLOAT = Integer.parseInt( | |||
| System.getProperty(BLOAT_PROPERTY, "100") | |||
There was a problem hiding this comment.
I think the druid planner we'll not like at all if multiple projects are kep on top of eachother...
the default seems to be the same (100) - what do you think about raising it to a bigger value; like a few millions - so that its less likely to hit this issue out of the box ?
There was a problem hiding this comment.
Hi, considering that this is primarily a workaround for a potential Calcite problem, I'm hesitant to increase the default value that much. However, I agree that raising the default to at least 1000 would be a good compromise.
There was a problem hiding this comment.
yeah - I agree...an off switch would be better than tweaking an integer;
it depends on the execution engine how it evaluates a set of expressions which may share common subexpressions ...it might not cause much problem in case it could avoid recomputations.
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java
Fixed
Show fixed
Hide fixed
| @@ -82,6 +83,10 @@ public class CalciteRulesManager | |||
| private static final int HEP_DEFAULT_MATCH_LIMIT = Integer.parseInt( | |||
| System.getProperty(HEP_DEFAULT_MATCH_LIMIT_CONFIG_STRING, "1200") | |||
| ); | |||
| private static final String BLOAT_PROPERTY = "druid.sql.planner.bloat"; | |||
| private static final int BLOAT = Integer.parseInt( | |||
There was a problem hiding this comment.
It should be passed via context parameter or through runtime property. A context param seems a better fit as it would allow you to run those complex queries without having any impact on the rest of the queries.
There was a problem hiding this comment.
Good point, thanks! I will have a look
There was a problem hiding this comment.
Updated to use context parameter
| */ | ||
| private static final List<RelOptRule> BASE_RULES = | ||
| ImmutableList.of( | ||
| CoreRules.AGGREGATE_STAR_TABLE, | ||
| CoreRules.AGGREGATE_PROJECT_STAR_TABLE, | ||
| CoreRules.PROJECT_MERGE, | ||
| ProjectMergeRule.Config.DEFAULT.withBloat(BLOAT).toRule(), |
There was a problem hiding this comment.
How do users know when to set this context parameter ?
I think we should change the error message so that they can be a bit more helpful for the end users.
There was a problem hiding this comment.
Honestly, these are calcite internals, and all we get on the druid side is RelOptPlanner.CannotPlanException, which I believe could be thrown by tons of reasons. We can add the suggestion to try to set this context parameter at QueryHandler, but IMO it's a real shot in the dark there.
Description
This pull request introduces a configurable
druid.sql.planner.bloatparameter in CalciteRulesManager.Fixed the problem when complicated nested sql queries which where working before Druid 29.0.0 (1.21.0 calcite version) are being rejected by calcite planner.
With exception:
There are not enough rules to produce a node with desired properties: convention=DRUID, sort=[]Appears that the exceptiom is being thrown, when the root
RelNodewhich initially is instance ofAbstractConverteris being treated asProjectMergeRule, which counts the result of merge not optimal and reject merging, which means that transformation of AbstractConverter is not happening, so at the end of the day thisAbstractConverterinstance keeps skipping bestCost initialization, because it returns default infinite value which is being skipped by VolcanoPlanner. That's why bestCost to root relNode is never set which is causing exceptionThis issue is probably caused by lots of optimizations in
VolcanoPlannerandProjectMergeRuleand looks like as a calcite bug.Release note
Created workaround: configurable
druid.sql.planner.bloatparameter, which should be set on brokerjvm.conf, it will adjust the limits ofProjectMergeRuleresult cost and could allow running complex queries(default value 100, suggested 1000, but it depends on data to be processed).Key changed/added classes in this PR
CalciteRulesManagerThis PR has: