fix: descend all legs for requireTimeCondition (#17407) by NishthaShah · Pull Request #20441 · apache/druid

NishthaShah · 2026-09-28T07:33:59Z

Description

requireTimeCondition resolved its answer from a single QuerySegmentSpec, so one unbounded leg could sneak past:

Pre-33: DataSourceAnalysis#getBaseQuerySegmentSpec returned the outer JoinDataSource spec (ETERNITY) — joins of two bounded subqueries were rejected (RequireTimeCondition does not handle complex joins #17407).
v33 through feat: sqlTimeCondition context parameter. #20607: ExecutionVertex#getEffectiveQuerySegmentSpec was pruned at mayCollapseQueryDataSource (default false), so an outer QueryDataSource hid every inner bounded scan. feat: sqlTimeCondition context parameter. #20607 fixed that specific shape by stopping traversal at non-collapsible subqueries; join-of-bounded-subqueries and multi-leg joins remained incorrect.

Fix

A new RequireTimeConditionAnalyzer (org.apache.druid.query.planning) walks the full tree via ExecutionVertexShuttle and returns true iff every physical leaf has an ancestor query with a bounded interval. It lives outside ExecutionVertex because getEffectiveQuerySegmentSpec is load-bearing for execution (segment selection, walker lookup, intervals) and this guard is pure validation.

Walker invariants:

Descends into every QueryDataSource with no mayCollapseQueryDataSource gate — covers non-collapsible subquery stacks.
Descends into both inputs of every JoinDataSource; an unbounded right-leg physical scan is rejected (Position.JOIN_RIGHT). GlobalTableDataSource on the right is exempt because JoinableFactory materializes it as an IndexedTable instead of scanning segments.
Keys on query intervals, not filters — a __time predicate over a subquery result stays a residual filter and must not satisfy the guard.

Only NativeQueryMaker is touched; MSQTaskQueryMaker, DartQueryMaker, and PrePlannedDartQueryMaker already skip this guard.

Release note

Fixed a bug in druid.sql.planner.requireTimeCondition where queries with a __time filter on every physical leg could be rejected as if they had no filter (joins of two bounded subqueries, and — before #20607 — non-collapsible subquery stacks over bounded scans).

The guard is now applied to every physical leaf, not just the outermost scan. Queries that previously passed with only an outer __time filter will now be rejected if an inner leg scans the full datasource:

A right-hand join input or semi-join subquery with no __time filter, even when the left-hand side is bounded.
A nested subquery whose innermost query ancestor has no __time filter, even when an outer wrapper is bounded.

GlobalTableDataSource on the right side of a join is exempt.

Key changed/added classes in this PR

RequireTimeConditionAnalyzer (new)
NativeQueryMaker
CalciteQueryTest
EmbeddedRequireTimeConditionTest

This PR has:

been self-reviewed.
added unit tests covering new code paths.

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

🟡 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.

FrankChen021 · 2026-10-05T12:24:47Z

+            crossedJoinLegBoundary = true;
+          }
+        }
+        if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {


[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.

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").

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

🟡 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.

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

🟡 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.

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.

FrankChen021

🟢 Approval recommended

No actionable issues found in this review. The innermost-query interval check fixes the previously reported bounded-outer/unbounded-inner bypass, including nested groupBy, while preserving bounded inner scans and independent bounded right join legs. The earlier interval-attribution findings are resolved in the reviewed head.

Reviewed 4 of 4 changed files. Started with the 2 incremental files, then inspected the full current diff and surrounding query traversal, subquery materialization, nested groupBy execution, native-query enforcement, and SQL test verification.

Validation: full and incremental git diff --check passed at head 459eb6890641be69b0448db507054ea1d7357281. Static review only; no tests or builds were run.

This is an automated review by Codex GPT-5.6-Luna(max)

…n error Expose a Result record from RequireTimeConditionAnalyzer carrying the first offending physical leaf's datasource name and its structural position (TOP_LEVEL, NESTED_SUBQUERY, JOIN_RIGHT). NativeQueryMaker composes a position-specific message so the user sees which table and roughly where the __time filter is missing, instead of a single generic string.

…onAnalyzer Add four tests exercising the walker at 4-5 layers deep: - Two pure-subquery depth-4 cases (bound at innermost / bound only in middle) confirm that only the innermost owning query's segment spec reaches the physical scan. - Two mixed subquery-and-join cases confirm the same at depth with a join in the chain: three bounded subquery wrappers above a Join with a bare unbounded right leg reject at the right-leg boundary (JOIN_RIGHT); a five-layer mix with a bounded right-leg wrapper above an unbounded inner rejects at the inner query (NESTED_SUBQUERY).

…guard Adds testRequireTimeConditionNestedRollupPositive to pin a common nested GROUP BY shape at the SQL layer: an outer aggregate wrapping an inner subquery whose GROUP BY owns the __time bound. The analyzer-level coverage for this shape already exists via testIntermediateBoundedInnerUnderUnboundedOuterIsSatisfied; this test adds the end-to-end SQL assertion so the planner path stays regression-free.

Adds two tests exercising the CTE (WITH ... AS (...)) surface: - testRequireTimeConditionCtePositive: a CTE that owns the __time bound in its definition inlines to a bounded inner subquery under the outer aggregate; the guard must accept it. - testRequireTimeConditionCteNegative: a CTE with no __time filter anywhere inlines to an unbounded scan; the guard must reject with the "__time column" message. Calcite inlines CTEs into equivalent nested subqueries (and unions when a CTE is referenced more than once), so these shapes already map onto existing analyzer coverage. These tests pin the SQL-layer behaviour so WITH syntax is not an unaudited user-visible dimension.

@test

…configs Converts the nine testRequireTimeCondition*Positive* tests to JUnit 5 @ParameterizedTest, running each case under both PLANNER_CONFIG_REQUIRE_TIME_CONDITION and PLANNER_CONFIG_DEFAULT. Each test executes 2 invocations (18 total) and asserts that enabling the guard does not change the native plan or the result rows for compliant queries. Scope: - testRequireTimeConditionPositive - testRequireTimeConditionPositive2 - testRequireTimeConditionPositive3 - testRequireTimeConditionLogicalValuePositive - testRequireTimeConditionExplicitJoinBothSidesFilteredPositive - testRequireTimeConditionLookupRightSidePositive - testRequireTimeConditionNonCollapsibleSubqueryPositive - testRequireTimeConditionNestedRollupPositive - testRequireTimeConditionCtePositive Negatives remain plain @test with assertThrows: under require-off they would plan as normal unbounded queries, which already has coverage elsewhere and offers weak incremental signal.

FrankChen021

🟢 Approval recommended

No actionable issues found in this review. The current analyzer, native-query integration, diagnostic result handling, and planner-configuration test coverage are consistent with the intended requireTimeCondition behavior.

Reviewed 4 of 4 changed files. Started with the incremental diff, then inspected the full current diff and surrounding query/datasource traversal and SQL enforcement paths.

Validation: full and incremental git diff --check passed. Static review only; no tests or builds were run.

This is an automated review by Codex GPT-5.6-Luna(max)

@test

…m recording DecoupledPlanningCalciteQueryTest pins the native plan for this semi-join via a frozen quidem .iq file keyed on the test's display name. Parametrising the method made the harness look for per-config recordings (testRequireTimeConditionPositive3@require-on.iq etc.) that don't exist, breaking DecoupledPlanningCalciteQueryTest in CI. Revert this one test to plain @test. The guard runs post-plan, so the matrix adds no planner coverage here anyway. The other 8 positives stay parametrised.

Pins the end-to-end behaviour of the guard through the SQL planner + Avatica path on two positive shapes (bounded nested-rollup, bounded CTE) and one negative (top-level unbounded) that asserts the formatted error message. Complements the existing CalciteQueryTest coverage: those tests exercise the guard against the native query tree directly, while this one validates the plan + results + error-message flow a JDBC client actually sees. Also pins the mysql-formatted result tables, which protects against regressions in planning that would otherwise silently change what the guard sees under it.

Boots an in-process cluster with druid.sql.planner.requireTimeCondition=true, ingests a datasource via the standard BASIC_INDEX task, and submits SQL through the Broker's HTTP endpoint. Verifies both the positive path (bounded query returns results) and the negative path (unbounded query is rejected via a 400 Bad Request carrying the "has no filter on the __time column" message). This complements the existing coverage: - RequireTimeConditionAnalyzerTest -- analyzer semantics (unit) - CalciteQueryTest testRequireTimeCondition* -- planner-level SQL guard (unit) - requireTimeCondition.iq -- quidem SQL regression recording and closes the loop end-to-end through broker config wiring and the Avatica/HTTP response path.

FrankChen021

🟡 Changes recommended

Fix the global-datasource shortcut before merging. It currently exempts physical broadcast tables from requireTimeCondition, allowing direct or left-side unbounded scans to bypass the guard.

Reviewed 6 of 6 changed files.

Validation: full and incremental git diff --check passed. Static review only; no builds or tests 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.

… datasources GlobalTableDataSource extends TableDataSource and remains segment-backed when queried directly or on a join's left leg (see its javadoc). The blanket dataSource.isGlobal() exemption at the leaf check therefore let an unbounded scan of a broadcast datasource bypass requireTimeCondition. Restrict the leaf-level exemption to leaves that are not instanceof TableDataSource (lookups, inline, frame-based inline), and preserve the right-leg broadcast-join path by short-circuiting isGlobal() leaves inside the JOIN_RIGHT branch (where a GlobalTableDataSource is materialized via JoinableFactory as an IndexedTable rather than scanned as segments). Adds four analyzer-level regression tests covering direct (bounded + eternity), left-leg under an unbounded outer, and right-leg broadcast-join cases for GlobalTableDataSource.

FrankChen021

🟢 Approval recommended

No actionable issues found in this review. The current head fixes the GlobalTableDataSource exemption on direct and left-join scan paths while preserving valid global right-join behavior.

Reviewed 6 of 6 changed files.

Validation: full and incremental diff checks passed. Static review only; no builds or tests were run.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021 · 2026-10-30T10:52:38Z

Thanks for iterating — the analyzer logic looks right now. Before merging:

Rebase onto master. feat: sqlTimeCondition context parameter. #20607 changed the same check (getBaseIntervals(), plus ExecutionVertexExplorer no longer traversing non-collapsible subqueries). Please have the analyzer replace getBaseIntervals(), re-verify the new tests against feat: sqlTimeCondition context parameter. #20607, and update the description — master may already accept the minimal repro.
Document the stricter behavior. Unbounded right-side join inputs / semi-join subqueries (JOIN_RIGHT) are now rejected, whereas master accepts them when the left side is bounded. Please note this in the release note and in docs/configuration/index.md.
Minor: the class Javadoc says right-hand join inputs are "not descended", but the walker does descend and reject them — please fix the comment.

# Conflicts: # sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java

Expand the config reference and add release-note + upgrade-note entries calling out that unbounded right-leg join inputs, semi-join subqueries, and unbounded inner subqueries are now rejected even when the outer query is bounded.

The walker does descend into right-hand join inputs and rejects an unbounded right-leg physical leaf with Position.JOIN_RIGHT, so the earlier "intentionally not descended" wording was stale.

…quireTimeConditionTest Switch from configuring the broker-level `druid.sql.planner.requireTimeCondition` property to passing `requireTimeCondition=true` in the SQL query context so the test exercises the per-query enablement path (which takes precedence over the broker default).

FrankChen021

🟢 Approval recommended

The requested rebase, documentation, and Javadoc updates are present, and the analyzer's per-leaf interval and right-leg handling remains consistent with execution semantics. One non-blocking test-coverage gap remains: the embedded test now exercises query-context enablement but no longer exercises broker-level property loading; runtime behavior is unaffected and this can be addressed afterward.

Reviewed 8 of 8 changed files. Started with the incremental diff, then reviewed the full current diff and surrounding query/datasource traversal.

Validation: git diff --check d2c015085816174151a08f0fc3040c240ebf4b01..c3600af8b20eb6ddc5fdf2ceb953c9d151350bc5 passed. Static review only; no tests or builds were run.

Severity	Findings
P0	0
P1	0
P2	0
P3	1
Total	1

[P3] Retain broker-level configuration coverage — embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/EmbeddedRequireTimeConditionTest.java:42
- Finding: This revision removes the broker-level requireTimeCondition setup and only sends requireTimeCondition=true in each request. The embedded suite no longer exercises the documented broker-level setting, so a regression in loading that property could pass this test.
- Suggestion: Keep a broker-configured case, or add separate embedded coverage, alongside the per-query override test.

This is an automated review by Codex GPT-5

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

Severity	Findings
P0	0
P1	0
P2	0
P3	1
Total	1

FrankChen021 · 2026-10-31T00:26:20Z

+ */
+public class EmbeddedRequireTimeConditionTest extends QueryTestBase
+{
+  private static final Map<String, Object> REQUIRE_TIME_CONDITION_CONTEXT =


[P3] Retain broker-level configuration coverage

Finding: This revision removes the broker-level requireTimeCondition setup and only sends requireTimeCondition=true in each request. The embedded suite no longer exercises the documented broker-level setting, so a regression in loading that property could pass this test.

Suggestion: Keep a broker-configured case, or add separate embedded coverage, alongside the per-query override test.

FrankChen021 · 2026-11-02T10:49:06Z

Release note: docs/release-info/release-notes.md is a template filled in by the release manager, so please revert it and move that text into the "Release note" section of the PR description. That section currently only mentions the bug fix, not the stricter enforcement for right-side join inputs and inner subqueries.
PR description: it still says the minimal repro is rejected on master and that right-hand join inputs aren't checked. Please update it to match the current behavior after feat: sqlTimeCondition context parameter. #20607.
EmbeddedRequireTimeConditionTest: PlannerConfig.CTX_REQUIRE_TIME_CONDITION is deprecated; please use QueryContextParameters.REQUIRE_TIME_CONDITION. Also, the test now enables the check through the query context instead of the Broker property, so it no longer covers druid.sql.planner.requireTimeCondition end to end. Could you keep a case that uses the Broker property?

… in EmbeddedRequireTimeConditionTest Addresses PR review feedback on apache#20441: - Swap deprecated PlannerConfig.CTX_REQUIRE_TIME_CONDITION for QueryContextParameters.REQUIRE_TIME_CONDITION in the embedded test. - Re-enable the guard via the Broker property druid.sql.planner.requireTimeCondition=true so the end-to-end property path is covered again; add testQueryContextOverrideCanDisableGuardPerQuery to still exercise the per-query context override. - Revert docs/release-info/release-notes.md to the master template; the stricter-enforcement text now lives in the PR description.

NishthaShah · 2026-11-02T23:52:47Z

Done. Thanks a lot for the review @FrankChen021

3. EmbeddedRequireTimeConditionTest: PlannerConfig.CTX_REQUIRE_TIME_CONDITION is deprecated; please use QueryContextParameters.REQUIRE_TIME_CONDITION. Also, the test now enables the check through the query context instead of the Broker property, so it no longer covers druid.sql.planner.requireTimeCondition end to end. Could you keep a case that uses the Broker property?

For the above I had the changes in local since some runs, but missed to push it on the PR.

Addressed all of the comments, could you please help take a look again?

FrankChen021

🟢 Approval recommended

The analyzer, SQL integration, configuration coverage, and regression tests are consistent with the intended per-leaf enforcement. One non-blocking documentation mismatch remains: the configuration reference omits the exemption for globally materialized right-side datasources; runtime behavior is unaffected, so merging can proceed.

Reviewed 7 of 7 changed files. Started with the incremental diff, then reviewed the full current diff and surrounding traversal, interval selection, global datasource handling, native SQL enforcement, and embedded configuration lifecycle.

Validation: git diff --check d2c015085816174151a08f0fc3040c240ebf4b01..3beba3a12a062aca673e7a137cb53ff658816f48 passed; git diff --check c3600af8b20eb6ddc5fdf2ceb953c9d151350bc5..3beba3a12a062aca673e7a137cb53ff658816f48 passed. Static review only; no tests or builds were run.

Severity	Findings
P0	0
P1	0
P2	0
P3	1
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.

FrankChen021 · 2026-11-03T12:54:47Z

 |`druid.sql.planner.useApproximateTopN`|Whether to use approximate [TopN queries](../querying/topnquery.md) when a SQL query could be expressed as such. If false, exact [GroupBy queries](../querying/groupbyquery.md) will be used instead.|true|
 |`druid.sql.planner.useLexicographicTopN`|Whether to use [TopN queries](../querying/topnquery.md) with lexicographic dimension ordering. If false, [GroupBy queries](../querying/groupbyquery.md) will be used instead for lexicographic ordering. When both this and `useApproximateTopN` are false, TopN queries are never used.|false|
-|`druid.sql.planner.requireTimeCondition`|Whether to require SQL to have filter conditions on `__time` column so that all generated native queries will have user specified intervals. If true, all queries without filter condition on `__time` column will fail|false|
+|`druid.sql.planner.requireTimeCondition`|Whether to require SQL to have filter conditions on `__time` column so that all generated native queries will have user specified intervals. If true, all queries without a filter condition on `__time` are rejected. The guard inspects every physical leaf: a right-hand join input or semi-join subquery with no `__time` filter is also rejected, even when the left-hand side is bounded. A time condition on the outer query does not satisfy an unbounded inner subquery or an unbounded right-leg join input.|false|


[P3] Document the global right-join exemption

Finding: The updated configuration reference says an unbounded right-hand join input or semi-join subquery is rejected, but the analyzer intentionally exempts globally materialized right-side datasources such as lookups and GlobalTableDataSource. This can mislead users into expecting valid global joins to fail or trying to add an impossible __time filter.

Suggestion: Mention the global right-side datasource exemption so the description matches the analyzer, tests, and PR release-note text.

github-actions Bot added the Area - Querying label Sep 28, 2026

NishthaShah marked this pull request as draft September 28, 2026 07:39

NishthaShah added 2 commits September 28, 2026 23:21

NishthaShah marked this pull request as ready for review October 5, 2026 09:59

FrankChen021 reviewed Oct 5, 2026

View reviewed changes

FrankChen021 reviewed Oct 7, 2026

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java Resolved

FrankChen021 reviewed Oct 8, 2026

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java Resolved

FrankChen021 reviewed Oct 9, 2026

View reviewed changes

NishthaShah added 3 commits October 12, 2026 10:51

NishthaShah force-pushed the fix/17407-require-time-condition-join branch from 93a992d to b296878 Compare October 12, 2026 08:52

NishthaShah added 2 commits October 12, 2026 14:33

FrankChen021 reviewed Oct 12, 2026

View reviewed changes

NishthaShah added 3 commits October 12, 2026 17:49

FrankChen021 reviewed Oct 13, 2026

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java Resolved

FrankChen021 reviewed Oct 14, 2026

View reviewed changes

abhishekrb19 mentioned this pull request Oct 16, 2026

feat: sqlTimeCondition context parameter. #20607

Merged

FrankChen021 added the Approval Recommended Used when AI completes a review and reports no findings label Oct 19, 2026

github-actions Bot added the Area - Documentation label Oct 30, 2026

NishthaShah added 3 commits October 30, 2026 18:30

Merge commit 'd2c0150858' into fix/17407-require-time-condition-join

cfe93a1

# Conflicts: # sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java

docs: correct RequireTimeConditionAnalyzer javadoc on right-leg handling

7fe4396

The walker does descend into right-hand join inputs and rejects an unbounded right-leg physical leaf with Position.JOIN_RIGHT, so the earlier "intentionally not descended" wording was stale.

NishthaShah force-pushed the fix/17407-require-time-condition-join branch from 52b2c0f to 7fe4396 Compare October 30, 2026 13:39

FrankChen021 reviewed Oct 31, 2026

View reviewed changes

FrankChen021 approved these changes Nov 3, 2026

View reviewed changes

FrankChen021 reviewed Nov 3, 2026

View reviewed changes

FrankChen021 merged commit 2e521fd into apache:master Nov 4, 2026
27 checks passed

github-actions Bot added this to the 40.0.0 milestone Nov 4, 2026

Conversation

NishthaShah commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fix

Release note

Key changed/added classes in this PR

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

FrankChen021 Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

NishthaShah Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Oct 7, 2026

Choose a reason for hiding this comment

Follow-up assessment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

FrankChen021 commented Oct 30, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

🟢 Approval recommended

Uh oh!

FrankChen021 Oct 31, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 commented Nov 2, 2026

Uh oh!

NishthaShah commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

FrankChen021 Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

NishthaShah commented Sep 28, 2026 •

edited

Loading

NishthaShah commented Nov 2, 2026 •

edited

Loading