Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature configurable calcite bloat #16248

Merged
merged 5 commits into from
May 6, 2024
Merged

Feature configurable calcite bloat #16248

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f67e86
Configurable bloat for calcite ProjectMergeRule implemented
Apr 8, 2024
368a655
Comment added
Apr 9, 2024
9f599ee
Default bloat value increased to 1000
Apr 10, 2024
f4c9109
Implemented bloat configuration from QueryContext
Apr 22, 2024
8cd5f37
Code refactored, docs updated
Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.calcite.rel.rules.CoreRules;
import org.apache.calcite.rel.rules.DateRangeRules;
import org.apache.calcite.rel.rules.JoinPushThroughJoinRule;
import org.apache.calcite.rel.rules.ProjectMergeRule;
import org.apache.calcite.rel.rules.PruneEmptyRules;
import org.apache.calcite.sql.SqlExplainFormat;
import org.apache.calcite.sql.SqlExplainLevel;
Expand Down Expand Up @@ -82,6 +83,10 @@
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks! I will have a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use context parameter

System.getProperty(BLOAT_PROPERTY, "1000")
);
Fixed Show fixed Hide fixed

/**
* Rules from {@link org.apache.calcite.plan.RelOptRules#BASE_RULES}, minus:
Expand All @@ -95,12 +100,16 @@
* and {@link CoreRules#FILTER_INTO_JOIN}, which are part of {@link #FANCY_JOIN_RULES}.
* 4) {@link CoreRules#PROJECT_FILTER_TRANSPOSE} because PartialDruidQuery would like to have the Project on top of the Filter -
* this rule could create a lot of non-useful plans.
*
* {@link CoreRules#PROJECT_MERGE} includes configurable bloat parameter, as a workaround for Calcite exception
* (there are not enough rules to produce a node with desired properties) thrown while running complex sql-queries with
* big amount of subqueries. `druid.sql.planner.bloat` should be set in broker's `jvm.config` file.
*/
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(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CoreRules.FILTER_SCAN,
CoreRules.FILTER_PROJECT_TRANSPOSE,
CoreRules.JOIN_PUSH_EXPRESSIONS,
Expand Down
Loading