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
+71 −1
Merged

Feature configurable calcite bloat #16248

Changes from 1 commit
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
Prev Previous commit
Next Next commit
Comment added
  • Loading branch information
sviatahorau committed Apr 9, 2024
commit 368a655fb54adcee5f0fe204f4fd7940eac098d5
4 changes: 4 additions & 0 deletions 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 @@ -84,9 +84,9 @@
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, "100")
Copy link
Member

@kgyrtkirk kgyrtkirk Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

);

/**
* Rules from {@link org.apache.calcite.plan.RelOptRules#BASE_RULES}, minus:
Expand All @@ -100,6 +100,10 @@
* 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(
Expand Down
Loading