aho135 commented on code in PR #20314:
URL: https://github.com/apache/druid/pull/20314#discussion_r4000658023
##########
sql/src/main/java/org/apache/druid/sql/DirectStatement.java:
##########
@@ -196,20 +199,52 @@ public ResultSet plan()
}
long planningStartNanos = System.nanoTime();
try (DruidPlanner planner = createPlanner()) {
- validate(planner);
- authorize(planner, authorizer());
+ // Bound the wall-clock time spent planning this query. A non-positive
timeout disables this. The budget is
+ // measured from planningStartNanos (above), so any time already spent
constructing the planner counts against
+ // it and a query cannot get a fresh full budget after an expensive
planner/schema setup.
+ final long maxPlanningTimeMs =
planner.getPlannerContext().getPlannerConfig().getMaxPlanningTimeMs();
+ final long remainingBudgetMs =
remainingPlanningBudgetMs(maxPlanningTimeMs, planningStartNanos);
Review Comment:
Fixed in c232e09a77. Two changes in `plan()`:
1. After `createPlanner()` returns, if the elapsed time since
`planningStartNanos` (which includes planner/schema construction) has already
reached `maxPlanningTimeMs`, throw `QueryTimeoutException` immediately — before
arming the watchdog or doing any further work. So a slow construction can no
longer consume the whole budget uninterrupted.
2. Removed the `Math.max(1, …)` floor in `remainingPlanningBudgetMs` (it's
the source of the 1ms schedule); since we now reject an already-exhausted
budget up front, the watchdog is always armed with a strictly positive
remaining budget. On the success path I also re-check the wall-clock deadline
in addition to `isTimedOut()`, so a plan that finishes after the deadline
(watchdog not yet fired due to scheduler jitter) is still rejected.
Added `SqlStatementTest#testPlanningTimeoutDuringPlannerConstruction`, which
sleeps past the budget inside an overridden `createPlanner()` and asserts the
timeout.
##########
sql/src/main/java/org/apache/druid/sql/DirectStatement.java:
##########
@@ -196,20 +197,44 @@ public ResultSet plan()
}
long planningStartNanos = System.nanoTime();
try (DruidPlanner planner = createPlanner()) {
- validate(planner);
- authorize(planner, authorizer());
+ // Bound the wall-clock time spent planning this query. A non-positive
timeout disables this.
+ final long maxPlanningTimeMs =
planner.getPlannerContext().getPlannerConfig().getMaxPlanningTimeMs();
+ try (SqlPlanningTimeout timeout = SqlPlanningTimeout.arm(
Review Comment:
Addressed the remaining planner-construction gap in c232e09a77 — see the
reply on the newer thread. The deadline is now enforced against elapsed time
(including `createPlanner()`) before planning proceeds, and re-checked on the
success path, so an exhausted budget is reported as a timeout rather than
reduced to a 1ms schedule.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java:
##########
@@ -303,12 +308,44 @@
cleanupRules.addRuleInstance(CoreRules.PROJECT_MERGE);
cleanupRules.addRuleInstance(AggregateProjectMergeRule.Config.DEFAULT.toRule());
return Programs.sequence(
- Programs.of(builder.build(), true,
DefaultRelMetadataProvider.INSTANCE),
+ hepProgram(builder.build(), plannerContext),
new DruidTrimFieldsProgram(),
- Programs.of(cleanupRules.build(), true,
DefaultRelMetadataProvider.INSTANCE)
+ hepProgram(cleanupRules.build(), plannerContext)
);
}
+ /**
+ * Equivalent to {@link Programs#of(HepProgram, boolean,
org.apache.calcite.rel.metadata.RelMetadataProvider)} with
+ * {@code noDag=true} and {@link DefaultRelMetadataProvider}, except the
{@link HepPlanner} is given a
+ * {@link org.apache.calcite.plan.Context} carrying the per-query {@link
org.apache.calcite.util.CancelFlag}.
+ * {@code Programs.of} builds its {@code HepPlanner} with a {@code null}
context, so it never observes our cancel
+ * flag; a query that spends its planning budget in these Hep rule loops
would otherwise ignore the planning timeout.
+ */
+ private static Program hepProgram(final HepProgram hepProgram, final
PlannerContext plannerContext)
+ {
+ return (planner, rel, requiredOutputTraits, materializations, lattices) ->
{
+ final HepPlanner hepPlanner = new HepPlanner(
+ hepProgram,
+ Contexts.of(plannerContext.getCancelFlag()),
+ true,
+ null,
+ RelOptCostImpl.FACTORY
+ );
+ final List<RelMetadataProvider> metadataProviders = new ArrayList<>();
+ metadataProviders.add(DefaultRelMetadataProvider.INSTANCE);
+ hepPlanner.registerMetadataProviders(metadataProviders);
Review Comment:
Fixed in c232e09a77 — dropped the deprecated
`HepPlanner#registerMetadataProviders` call. Modern Calcite routes rule
metadata through the cluster provider, which the helper already sets via
`cluster.setMetadataProvider(...)` (matching `Programs.of`); the
decoupled/join/array/select Calcite query tests pass unchanged, confirming the
metadata behavior is preserved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
