aho135 commented on code in PR #20314:
URL: https://github.com/apache/druid/pull/20314#discussion_r3984487462
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java:
##########
@@ -220,6 +221,10 @@ public SqlConformance conformance()
if (aClass.equals(PlannerContext.class)) {
return (C) plannerContext;
}
+ if (aClass.equals(CancelFlag.class)) {
Review Comment:
Fixed in fd86a12573. You're right that `Programs.of` builds its `HepPlanner`
with a null `Context`, so the per-query `CancelFlag` never reached the
pre/reduction/pre-Volcano/cleanup Hep stages (the reduction stage is where
`RexSimplify`/CALCITE-3178 runs). I added `CalciteRulesManager#hepProgram`,
which constructs the `HepPlanner` with
`Contexts.of(plannerContext.getCancelFlag())` and is otherwise identical to
`Programs.of(hepProgram, true, DefaultRelMetadataProvider.INSTANCE)`, and
switched those stages to use it. The cost-based Volcano stages already inherit
the flag via the cluster planner's context. 445
`DecoupledPlanningCalciteQueryTest` cases pass unchanged, confirming the helper
is behavior-preserving.
##########
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(
+ maxPlanningTimeMs,
+ planner.getPlannerContext().getCancelFlag(),
Review Comment:
Fixed in fd86a12573. Since `TableMacro.apply()` gives no handle to the outer
context, I added `PlannerContext#withInheritedCancelFlag`:
`DirectStatement#plan()` installs the top-level query's `CancelFlag` on a
thread-local for the duration of planning, and any `PlannerContext` created on
that thread (including the nested planner in `DruidViewMacro`) inherits it
instead of creating its own. So a single planning timeout now governs view
expansion too. (Interrupt remains advisory, as you note, but the shared flag
reaches the nested Volcano/Hep planners.)
##########
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(
+ maxPlanningTimeMs,
+ planner.getPlannerContext().getCancelFlag(),
+ Thread.currentThread()
+ )) {
+ try {
+ validate(planner);
+ authorize(planner, authorizer());
- // Adding the statement to the lifecycle manager allows cancellation.
- // Tests cancel during this call; real clients might do so if the plan
- // or execution prep stages take too long for some unexpected reason.
- sqlToolbox.sqlLifecycleManager.add(sqlQueryId(), this);
- transition(State.PREPARED);
- resultSet = createResultSet(createPlan(planner));
- prepareResult = planner.prepareResult();
- // Double check needed by SqlResourceTest
- transition(State.PREPARED);
- reporter.planningTimeNanos(System.nanoTime() - planningStartNanos);
- return resultSet;
+ // Adding the statement to the lifecycle manager allows cancellation.
+ // Tests cancel during this call; real clients might do so if the
plan
+ // or execution prep stages take too long for some unexpected reason.
+ sqlToolbox.sqlLifecycleManager.add(sqlQueryId(), this);
+ transition(State.PREPARED);
+ resultSet = createResultSet(createPlan(planner));
+ prepareResult = planner.prepareResult();
+ // Double check needed by SqlResourceTest
+ transition(State.PREPARED);
+ reporter.planningTimeNanos(System.nanoTime() - planningStartNanos);
+ return resultSet;
Review Comment:
Fixed in fd86a12573. `plan()` now re-checks `timeout.isTimedOut()` on the
success path (after `createPlan`/`prepareResult` return normally), not just in
the catch, and throws `QueryTimeoutException` if the watchdog had already fired
— so a plan completed in a non-cancellable section that swallowed the interrupt
is rejected rather than executed.
##########
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:
Fixed in fd86a12573. The watchdog is now armed with the remaining budget
measured from `planningStartNanos` (`remainingPlanningBudgetMs` =
`maxPlanningTimeMs` minus the time already elapsed, floored at 1ms), so
planner/schema construction counts against the budget and a query can't get a
fresh full timeout after an expensive setup.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java:
##########
@@ -205,6 +234,7 @@ public boolean equals(Object o)
&& useNativeQueryExplain == that.useNativeQueryExplain
&& forceExpressionVirtualColumns ==
that.forceExpressionVirtualColumns
&& maxNumericInFilters == that.maxNumericInFilters
+ && maxPlanningTimeMs == that.maxPlanningTimeMs
Review Comment:
Good catch, fixed in fd86a12573. `getNonDefaultAsQueryContext()` now emits
`maxPlanningTimeMs` when non-default, so the defensive config<->context
equality check passes; added
`testMaxPlanningTimeMsRoundTripsThroughQueryContext` to `PlannerConfigTest` to
cover it.
--
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]
