Repository navigation
fix: descend all legs for requireTimeCondition (#17407) - #20441
NishthaShah wants to merge 6 commits into
Conversation
The requireTimeCondition guard has two defects in opposite directions: * Pre-33: joins of two bounded subqueries were rejected because DataSourceAnalysis.getBaseQuerySegmentSpec resolved the outer join query first, whose interval is ETERNITY. * v33+: non-collapsible subquery stacks are rejected because ExecutionVertex.getEffectiveQuerySegmentSpec prunes at Query.mayCollapseQueryDataSource (default false), so the inner query is never visited. Resolve the check outside ExecutionVertex with a walker that descends every QueryDataSource and the left input of every JoinDataSource. The right-hand input of a join is intentionally not followed (preserving testRequireTimeConditionSemiJoinNegative), and the guard keys on query intervals rather than filters (preserving rejection of outer __time filters over unbounded subqueries). Adds nine tests covering explicit joins, three-way joins, UNION ALL, lookups on the right side of a join, non-collapsible subqueries, and outer filters over unbounded subqueries.
…imitations)
Both testRequireTimeConditionUnionAllBothBranchesFilteredPositive and
testRequireTimeConditionUnionAllOneBranchMissingFilterNegative fail
under MSQ and Decoupled planners for reasons unrelated to the
requireTimeCondition fix:
* MSQ rejects UNION ALL between filtered scans at planning time
('SQL requires union between inputs that are not simple table
scans') before NativeQueryMaker's guard runs.
* Decoupled produces a top-level UnionQuery (not a BaseQuery); the
~20 ExecutionVertex.of(query) callsites upstream of the analyzer
throw 'Can't traverse a query[UnionQuery]!'.
The base-planner UNION ALL cases pass and the analyzer handles the
tree correctly — supporting UNION ALL under MSQ/Decoupled would
require orthogonal fixes to ExecutionVertexShuttle and MSQ's union
planner, out of scope for this PR.
The remaining 14 requireTimeCondition tests still cover both regression
shapes from the RCA: non-collapsible subquery stacks (v33+) and joins
of two bounded subqueries (pre-33).
…rage Seven direct-call tests exercise the analyzer's paths so the diff-coverage check passes in the processing module (the sql-module CalciteQueryTest coverage is not counted toward this file). Covers bounded/unbounded scans, nested QueryDataSource descent (v33+ regression), global datasource short-circuit (LookupDataSource, InlineDataSource), and JoinDataSource leg-boundary detection. Coverage on RequireTimeConditionAnalyzer$TimeFilterExplorer: 98% instruction, 85% branch (was 0% / 0% from processing tests).
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
Fix interval attribution before merging. The analyzer currently allows a bounded outer query to satisfy the guard for an unbounded nested physical scan, so requireTimeCondition can still permit a full table scan in a subquery stack.
Reviewed 4 of 4 changed files.
Validation: mvn -ntp -pl processing -Dtest=org.apache.druid.query.planning.RequireTimeConditionAnalyzerTest -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true -T1C test passed (7 tests); git diff --check edea417869a2f2fcacd3d1f81b7ef3776908092e d9bea4c4028722ac6ef97fc9a4373b27cf5981d7 passed.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| crossedJoinLegBoundary = true; | ||
| } | ||
| } | ||
| if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) { |
There was a problem hiding this comment.
[P1] Do not let an outer interval mask an unbounded inner scan
Finding: The loop keeps searching past an unbounded query and returns as soon as any outer ancestor is bounded. For a non-collapsible stack such as a bounded outer query over an ETERNITY inner query, the physical leaf is executed by the inner query with no time interval, but hasTimeFilterOnAllLegs returns true and NativeQueryMaker allows it. This lets requireTimeCondition permit the full scan it is meant to reject.
Suggestion: Evaluate the interval at the innermost query that owns the leaf, while still respecting join-leg boundaries, and add a regression test for a bounded outer query over an unbounded inner query.
There was a problem hiding this comment.
Thanks. Pushed a regression commit that adds two tests in RequireTimeConditionAnalyzerTest to cover the "bounded outer over unbounded inner" shape being called out:
Verified the fix end-to-end for the specific SQL query for (outer interval mask an unbounded inner scan)
SELECT COUNT(*) FROM wikipedia foo JOIN (SELECT page, __time FROM wikipedia LIMIT 10) bar ON foo.page = bar.page WHERE foo.__time >= TIMESTAMP '2015-01-01';
Native plan is Timeseries([2015-01-01/∞)) over Join(TableDataSource(wikipedia), QueryDataSource(ScanQuery(wikipedia, ETERNITY, LIMIT 10))) — the exact P1 shape.
- Pre-fix (ExecutionVertex.getEffectiveQuerySegmentSpec only): wrongly succeeds, returns 17. The outer [2015-01-01/∞) masks the unbounded inner scan.
- Post-fix (walker descends every join leg): correctly rejects with CannotBuildQueryException("requireTimeCondition is enabled, all queries must include a filter condition on the __time column").
There was a problem hiding this comment.
Follow-up assessment
Reviewed 4 of 4 changed files. The new tests cover the reported right-join leg and nested right-leg cases, including the SQL shape in your reply. One related case still reaches a bounded ancestor: a non-collapsible QueryDataSource over an ETERNITY inner query outside a join-right leg. Here crossedJoinLegBoundary remains false, so line 109 returns true for the bounded outer query even though the inner query owns the unbounded physical scan. Please add a regression test for this shape and stop the ancestor search at the inner execution boundary.
Validation: Static review; git diff --check origin/master...HEAD passed. Tests were not run.
…nAnalyzer Add two regression tests for the Codex P1 raised on apache#20441: * testJoinWithLimitWrappedUnboundedRightIsNotSatisfied — a LIMIT-wrapped QueryDataSource on the right leg of a join under a bounded outer scan; LIMIT blocks __time predicate push-down, so the inner leaf stays at ETERNITY. The walker must cross the join-leg boundary before reaching the bounded ancestor and reject. * testNestedJoinWithDeepUnboundedLegIsNotSatisfied — a nested Join(FOO, Join(FOO, BAR)) under a bounded outer scan; the deepest leg crosses two join-leg boundaries, exercising transitive descent. Both guard against the pre-fix behavior where ExecutionVertex.getEffectiveQuerySegmentSpec would return the outer interval and silently accept the unbounded inner scan.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The added tests cover the unbounded right join leg from the reported SQL shape, including nested join traversal. A separate non-collapsible QueryDataSource path remains: when the inner query has ETERNITY and an outer query is bounded without crossing a join-right boundary, crossedJoinLegBoundary stays false and this loop accepts the outer interval. The inner query still owns the physical scan, so requireTimeCondition can allow an unbounded scan.
Reviewed all 4 changed files: RequireTimeConditionAnalyzer.java, RequireTimeConditionAnalyzerTest.java, NativeQueryMaker.java, and CalciteQueryTest.java.
Validation: git diff --check origin/master...HEAD passed. Static review only; tests were not run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| crossedJoinLegBoundary = true; | ||
| } | ||
| } | ||
| if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) { |
There was a problem hiding this comment.
[P1] Do not let a bounded outer query mask an unbounded inner scan
Finding: When a non-collapsible QueryDataSource wraps an unbounded inner query outside a join-right leg, this loop continues past the inner query and returns true for a bounded outer query. For example, a bounded query over QueryDataSource(scan(table, ETERNITY)) leaves the physical table scan owned by the unbounded inner query, yet requireTimeCondition is accepted and permits the full scan.
Suggestion: Treat a non-collapsible QueryDataSource as an execution boundary and require the query owning each physical leaf to have a bounded segment spec; add a regression test for a bounded outer query over an ETERNITY inner query.
Addresses Frank's follow-up P1 on apache#20441. The walker was returning true for a bounded outer query over an unbounded inner QueryDataSource outside any join-right leg, because the ancestor loop climbed past the inner query (unbounded) and accepted the outer's bound as sufficient. The physical table scan is actually owned by the inner query, so the outer's bound does not reach the leaf unless the chain between leaf and outer is fully collapsible. Track collapsedChain while walking ancestors: for every Query past the innermost, &= q.mayCollapseQueryDataSource(). Only accept a bounded Query ancestor when the chain between the leaf and that ancestor has collapsed. The innermost Query owns the leaf directly and does not need a collapsibility check. Preserves existing behavior for GroupBy-over-GroupBy (where mayCollapseQueryDataSource() is true) and rejects Scan-over-Scan via QueryDataSource (where it is false). Covered by new native test RequireTimeConditionAnalyzerTest.testBoundedOuterOverUnboundedQueryDataSourceIsNotSatisfied. Existing CalciteQueryTest#testRequireTimeCondition* cases unchanged.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The scan-over-scan bypass from the previous review is fixed, but a bounded outer groupBy still satisfies the guard for an inner groupBy that scans over ETERNITY. Address the interval attribution for that collapsible query chain before merging.
Reviewed 4 of 4 changed files. Started with both incremental files, then reviewed the full current diff, native-query integration, existing and added tests, execution-vertex interval selection, broker segment selection, and nested groupBy execution.
Validation: full and incremental git diff --check passed at head 9e4d0e20945f101de79ab26eea019bdf6abd5ae8. Static review only; no tests or builds were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| // absorbed into q (or further up). Each hop needs the outer Query to | ||
| // mayCollapseQueryDataSource() its inner QueryDataSource; otherwise the | ||
| // inner query runs independently and the outer's bound does not reach the leaf. | ||
| collapsedChain &= q.mayCollapseQueryDataSource(); |
There was a problem hiding this comment.
[P1] Do not treat groupBy collapsibility as interval pushdown
Finding: The new condition still accepts a bounded outer GroupByQuery over an unbounded inner GroupByQuery: GroupByQuery.mayCollapseQueryDataSource() returns true for this pair, so collapsedChain stays true and the outer interval satisfies the guard. That flag does not mean the outer interval is applied to the physical scan. ExecutionVertexExplorer records the innermost query's segment spec, CachingClusteredClient uses that spec for segment selection, and GroupByQueryQueryToolChest.mergeGroupByResultsWithoutPushDown executes the inner query with its original intervals before processing its results through the outer query. For example, replacing both scans in the new regression test with groupBy(TABLE_FOO, ETERNITY) and groupBy(new QueryDataSource(inner), BOUNDED) still returns true while the table is scanned over ETERNITY. This leaves requireTimeCondition bypassable for nested groupBy queries, which the pre-PR guard rejected using the innermost segment spec.
Suggestion: Require a bound on the query that actually owns each physical scan, and add the equivalent groupBy-over-groupBy regression test rather than assuming mayCollapseQueryDataSource propagates intervals.
mayCollapseQueryDataSource() is a datasource-collapse signal, not an interval-propagation one. GroupByQuery returns true for it over a QueryDataSource(GroupByQuery), but at runtime the inner query is executed with its original intervals (see GroupByQueryQueryToolChest#mergeGroupByResultsWithoutPushDown) and an outer bound never reaches the physical scan. Replace the ancestor-chain walk with the innermost-owning-query rule: for each physical leaf, walk up to the first Query ancestor and check its bound. Stop at join right-leg boundaries since the outer's spec only applies to the primary (left) input. Adds adversarial coverage for the follow-up P1 raised on apache#20441 (bounded outer GroupBy over ETERNITY inner GroupBy) plus five scenarios locking down over-rejection and under-rejection edges: right leg with its own bounded Query, 3-layer QDS with only middle bounded, bounded inner under ETERNITY outer, and UnionDataSource with bounded/unbounded outer.
2 participants
Fixes #17407.
Description
druid.sql.planner.requireTimeConditionhas two defects in opposite directions, both stemming from howNativeQueryMaker#runQueryresolves "the intervals that apply to this query":DataSourceAnalysis#getBaseQuerySegmentSpecresolved the outerJoinDataSourcequery first, whose interval isETERNITY. That is the shape reported in RequireTimeCondition does not handle complex joins #17407.ExecutionVertex#getEffectiveQuerySegmentSpec, whose traversal is pruned atQuery#mayCollapseQueryDataSource(defaultfalse). So under aQueryDataSource, the inner query is never visited and the outerETERNITYspec is what the check sees.A minimal repro rejected on master today, although its only table access is bounded:
Fix: resolve requireTimeCondition outside
ExecutionVertexExecutionVertex#getEffectiveQuerySegmentSpecis load-bearing for execution — it feeds segment selection inCachingClusteredClient, walker lookup inBaseQuery, and interval handling inQueriesandBrokerQueryResource. Changing it would change which segments get scanned.requireTimeConditionis asking a different, purely-validation question ("is there a__timefilter anywhere that bounds the base scan?") which is not bounded by the execution vertex.A new
RequireTimeConditionAnalyzer(packageorg.apache.druid.query.planning) walks the query tree viaExecutionVertexShuttleand returnstrueiff every physical leaf datasource is descended from an ancestor query with a boundedQuerySegmentSpec(i.e. notIntervals.ONLY_ETERNITY). Global datasources (lookups, inline) short-circuit as satisfied.Key invariants preserved by the walker:
QueryDataSourcewith nomayCollapseQueryDataSourcegate — covers non-collapsible subquery stacks.JoinDataSourcebut not the right — a__timefilter on the right side does not bound the base table. PreservestestRequireTimeConditionSemiJoinNegative.__timepredicate over a subquery result stays a residual filter and never becomes aQuerySegmentSpec, so it must not satisfy the guardrail (testRequireTimeConditionOuterFilterOverUnboundedSubqueryNegative).Scope
NativeQueryMakeris the only enforcement site touched.MSQTaskQueryMaker,DartQueryMaker, andPrePlannedDartQueryMakerdo not currently checkrequireTimeConditionat all, so the flag is silently a no-op on those paths. Extending the check to those makers is out of scope for this PR.Release note
Fixed a bug in
druid.sql.planner.requireTimeConditionwhere queries with a__timefilter on every physical leg were rejected as if they had no filter. Affected shapes include joins of two bounded subqueries (pre-33 regression) and non-collapsible subquery stacks over bounded scans (33+ regression).Key changed/added classes in this PR
RequireTimeConditionAnalyzer(new)NativeQueryMakerCalciteQueryTestThis PR has:
CalciteQueryTest, spanning explicit joins, three-way joins, lookups on the right side of a join, non-collapsible subqueries, and outer filters over unbounded subqueries; all four pre-existing positive and three pre-existing negativerequireTimeConditiontests continue to pass).