You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "NishthaShah (via GitHub)" <gi...@apache.org> on 2026/09/28 07:34:02 UTC
[PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
NishthaShah opened a new pull request, #20441:
URL: https://github.com/apache/druid/pull/20441
Fixes #17407.
### Description
`druid.sql.planner.requireTimeCondition` has two defects in opposite directions, both stemming from how `NativeQueryMaker#runQuery` resolves "the intervals that apply to this query":
* **Pre-33**: joins of two bounded subqueries were rejected. `DataSourceAnalysis#getBaseQuerySegmentSpec` resolved the outer `JoinDataSource` query first, whose interval is `ETERNITY`. That is the shape reported in #17407.
* **v33+**: non-collapsible subquery stacks are now rejected. After #17726 / #17763 the check uses `ExecutionVertex#getEffectiveQuerySegmentSpec`, whose traversal is pruned at `Query#mayCollapseQueryDataSource` (default `false`). So under a `QueryDataSource`, the inner query is never visited and the outer `ETERNITY` spec is what the check sees.
A minimal repro rejected on master today, although its only table access is bounded:
```sql
SELECT COUNT(*) FROM (
SELECT dim1 FROM druid.foo WHERE __time >= '2000-01-01' LIMIT 10
)
```
#### Fix: resolve requireTimeCondition outside `ExecutionVertex`
`ExecutionVertex#getEffectiveQuerySegmentSpec` is load-bearing for execution — it feeds segment selection in `CachingClusteredClient`, walker lookup in `BaseQuery`, and interval handling in `Queries` and `BrokerQueryResource`. Changing it would change which segments get scanned. `requireTimeCondition` is asking a different, purely-validation question ("is there a `__time` filter anywhere that bounds the base scan?") which is not bounded by the execution vertex.
A new `RequireTimeConditionAnalyzer` (package `org.apache.druid.query.planning`) walks the query tree via `ExecutionVertexShuttle` and returns `true` iff every physical leaf datasource is descended from an ancestor query with a bounded `QuerySegmentSpec` (i.e. not `Intervals.ONLY_ETERNITY`). Global datasources (lookups, inline) short-circuit as satisfied.
Key invariants preserved by the walker:
1. Descends into every `QueryDataSource` with no `mayCollapseQueryDataSource` gate — covers non-collapsible subquery stacks.
2. Descends the left input of every `JoinDataSource` but not the right — a `__time` filter on the right side does not bound the base table. Preserves `testRequireTimeConditionSemiJoinNegative`.
3. Keys on query intervals, not filters — a `__time` predicate over a subquery result stays a residual filter and never becomes a `QuerySegmentSpec`, so it must not satisfy the guardrail (`testRequireTimeConditionOuterFilterOverUnboundedSubqueryNegative`).
#### Scope
`NativeQueryMaker` is the only enforcement site touched. `MSQTaskQueryMaker`, `DartQueryMaker`, and `PrePlannedDartQueryMaker` do not currently check `requireTimeCondition` at 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.requireTimeCondition` where queries with a `__time` filter on every physical leg were rejected as if they had no filter. Affected shapes include joins of two bounded subqueries (pre-33 regression), non-collapsible subquery stacks over bounded scans (33+ regression), and `UNION ALL` of two bounded queries.
<hr>
##### Key changed/added classes in this PR
* `RequireTimeConditionAnalyzer` (new)
* `NativeQueryMaker`
* `CalciteQueryTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added unit tests covering new code paths (nine new tests in `CalciteQueryTest`, spanning explicit joins, three-way joins, `UNION ALL`, 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 negative `requireTimeCondition` tests continue to pass).
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20441:
URL: https://github.com/apache/druid/pull/20441#discussion_r4258747980
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.TableDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+import javax.annotation.Nullable;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ *
+ * <p>On rejection the analyzer records the first offending physical leaf's
+ * datasource name and its structural position so callers can build a specific
+ * error message.
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ public enum Position
+ {
+ TOP_LEVEL,
+ NESTED_SUBQUERY,
+ JOIN_RIGHT
+ }
+
+ public static final class Result
+ {
+ private final boolean satisfied;
+ @Nullable
+ private final String leafName;
+ @Nullable
+ private final Position position;
+
+ private Result(boolean satisfied, @Nullable String leafName, @Nullable Position position)
+ {
+ this.satisfied = satisfied;
+ this.leafName = leafName;
+ this.position = position;
+ }
+
+ public boolean isSatisfied()
+ {
+ return satisfied;
+ }
+
+ @Nullable
+ public String getLeafName()
+ {
+ return leafName;
+ }
+
+ @Nullable
+ public Position getPosition()
+ {
+ return position;
+ }
+ }
+
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return a {@link Result} describing whether every physical leaf datasource
+ * in {@code query} is descended from an ancestor query with a bounded
+ * interval. If not, the result also carries the first offending
+ * leaf's name and its structural position. Global datasources
+ * (lookups, inline) always satisfy the check.
+ */
+ public static Result analyze(Query<?> query)
+ {
+ final TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ if (explorer.allLegsBounded) {
+ return new Result(true, null, null);
+ }
+ return new Result(false, explorer.firstOffenderName, explorer.firstOffenderPosition);
+ }
+
+ /**
+ * Boolean-only convenience over {@link #analyze(Query)} for callers that do
+ * not need the offending-leaf details.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ return analyze(query).isSatisfied();
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+ @Nullable
+ String firstOffenderName;
+ @Nullable
+ Position firstOffenderPosition;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
Review Comment:
[P1] Global table leaves bypass the time guard
**Finding:** Defect: the blanket isGlobal() exemption treats GlobalTableDataSource as a non-physical global, although it extends TableDataSource and still represents segment-backed data. Trigger: a direct query or left join leg uses a broadcast datasource represented by GlobalTableDataSource with an ETERNITY interval. Impact: requireTimeCondition accepts the query and permits an unbounded scan of the broadcast table, defeating the configured guardrail.
**Suggestion:** Restrict the exemption to non-segment global datasources such as lookups and inline data, while preserving the intended right-side join behavior, and add direct/left-leg GlobalTableDataSource regression coverage.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
Posted by "NishthaShah (via GitHub)" <gi...@apache.org>.
NishthaShah commented on code in PR #20441:
URL: https://github.com/apache/druid/pull/20441#discussion_r4206323146
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return true if every physical leaf datasource in {@code query} is
+ * descended from an ancestor query with a bounded interval. Global
+ * datasources (lookups, inline) always satisfy the check.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ return explorer.allLegsBounded;
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
+ return dataSource;
+ }
+ boolean crossedJoinLegBoundary = false;
+ for (int i = parents.size() - 1; i >= 0; i--) {
+ EVNode ancestor = parents.get(i);
+ if (!ancestor.isQuery() && ancestor.index != null && ancestor.index != 0 && i > 0) {
+ EVNode parentOfAncestor = parents.get(i - 1);
+ if (!parentOfAncestor.isQuery() && parentOfAncestor.dataSource instanceof JoinDataSource) {
+ crossedJoinLegBoundary = true;
+ }
+ }
+ if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {
Review 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")`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20441:
URL: https://github.com/apache/druid/pull/20441#discussion_r4183933130
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return true if every physical leaf datasource in {@code query} is
+ * descended from an ancestor query with a bounded interval. Global
+ * datasources (lookups, inline) always satisfy the check.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ return explorer.allLegsBounded;
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
+ return dataSource;
+ }
+ boolean crossedJoinLegBoundary = false;
+ for (int i = parents.size() - 1; i >= 0; i--) {
+ EVNode ancestor = parents.get(i);
+ if (!ancestor.isQuery() && ancestor.index != null && ancestor.index != 0 && i > 0) {
+ EVNode parentOfAncestor = parents.get(i - 1);
+ if (!parentOfAncestor.isQuery() && parentOfAncestor.dataSource instanceof JoinDataSource) {
+ crossedJoinLegBoundary = true;
+ }
+ }
+ if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {
Review 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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20441:
URL: https://github.com/apache/druid/pull/20441#discussion_r4206767732
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return true if every physical leaf datasource in {@code query} is
+ * descended from an ancestor query with a bounded interval. Global
+ * datasources (lookups, inline) always satisfy the check.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ return explorer.allLegsBounded;
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
+ return dataSource;
+ }
+ boolean crossedJoinLegBoundary = false;
+ for (int i = parents.size() - 1; i >= 0; i--) {
+ EVNode ancestor = parents.get(i);
+ if (!ancestor.isQuery() && ancestor.index != null && ancestor.index != 0 && i > 0) {
+ EVNode parentOfAncestor = parents.get(i - 1);
+ if (!parentOfAncestor.isQuery() && parentOfAncestor.dataSource instanceof JoinDataSource) {
+ crossedJoinLegBoundary = true;
+ }
+ }
+ if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {
Review 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.
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return true if every physical leaf datasource in {@code query} is
+ * descended from an ancestor query with a bounded interval. Global
+ * datasources (lookups, inline) always satisfy the check.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ return explorer.allLegsBounded;
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
+ return dataSource;
+ }
+ boolean crossedJoinLegBoundary = false;
+ for (int i = parents.size() - 1; i >= 0; i--) {
+ EVNode ancestor = parents.get(i);
+ if (!ancestor.isQuery() && ancestor.index != null && ancestor.index != 0 && i > 0) {
+ EVNode parentOfAncestor = parents.get(i - 1);
+ if (!parentOfAncestor.isQuery() && parentOfAncestor.dataSource instanceof JoinDataSource) {
+ crossedJoinLegBoundary = true;
+ }
+ }
+ if (ancestor.isQuery() && !crossedJoinLegBoundary && isBounded(ancestor.getQuery())) {
Review 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.
<!-- mergelens:review -->
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: descend all legs for requireTimeCondition (#17407) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20441:
URL: https://github.com/apache/druid/pull/20441#discussion_r4219000942
##########
processing/src/main/java/org/apache/druid/query/planning/RequireTimeConditionAnalyzer.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.planning;
+
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.query.BaseQuery;
+import org.apache.druid.query.DataSource;
+import org.apache.druid.query.JoinDataSource;
+import org.apache.druid.query.Query;
+import org.apache.druid.query.QueryDataSource;
+import org.apache.druid.query.spec.QuerySegmentSpec;
+
+/**
+ * Walks a query tree to decide whether every physical leaf datasource has an
+ * ancestor query with a bounded {@link QuerySegmentSpec}, for the purpose of
+ * enforcing {@code druid.sql.planner.requireTimeCondition}.
+ *
+ * <p>Unlike {@link ExecutionVertex#getEffectiveQuerySegmentSpec()}, which is
+ * pruned at the execution-vertex boundary (see
+ * {@link Query#mayCollapseQueryDataSource()}), this walker descends into every
+ * {@link QueryDataSource} and every left input of a {@link JoinDataSource}.
+ * {@code requireTimeCondition} asks a validation question ("is there a
+ * {@code __time} filter anywhere that bounds the base scan?") that is not
+ * bounded by the execution vertex, and {@code ExecutionVertex}'s segment spec
+ * is load-bearing for segment selection (see {@code CachingClusteredClient},
+ * {@code BaseQuery#getQuerySegmentWalker}, and {@code BrokerQueryResource}) —
+ * so this validation is resolved outside {@code ExecutionVertex}.
+ *
+ * <p>Right-hand inputs of joins are intentionally not descended: a
+ * {@code __time} filter there does not bound the base table (see
+ * {@code CalciteQueryTest#testRequireTimeConditionSemiJoinNegative}).
+ */
+public final class RequireTimeConditionAnalyzer
+{
+ private RequireTimeConditionAnalyzer()
+ {
+ }
+
+ /**
+ * @return true if every physical leaf datasource in {@code query} is
+ * descended from an ancestor query with a bounded interval. Global
+ * datasources (lookups, inline) always satisfy the check.
+ */
+ public static boolean hasTimeFilterOnAllLegs(Query<?> query)
+ {
+ TimeFilterExplorer explorer = new TimeFilterExplorer(query);
+ return explorer.allLegsBounded;
+ }
+
+ private static class TimeFilterExplorer extends ExecutionVertexShuttle
+ {
+ boolean allLegsBounded = true;
+
+ TimeFilterExplorer(Query<?> query)
+ {
+ traverse(query);
+ }
+
+ @Override
+ protected boolean mayTraverseQuery(Query<?> query)
+ {
+ return true;
+ }
+
+ @Override
+ protected boolean mayTraverseDataSource(EVNode node)
+ {
+ return true;
+ }
+
+ @Override
+ protected Query<?> visitQuery(Query<?> query)
+ {
+ return query;
+ }
+
+ @Override
+ protected DataSource visit(DataSource dataSource, boolean leaf)
+ {
+ if (!isPhysicalLeaf(dataSource) || dataSource.isGlobal()) {
+ return dataSource;
+ }
+ boolean crossedJoinLegBoundary = false;
+ boolean collapsedChain = true;
+ int queriesSeen = 0;
+ for (int i = parents.size() - 1; i >= 0; i--) {
+ EVNode ancestor = parents.get(i);
+ if (!ancestor.isQuery() && ancestor.index != null && ancestor.index != 0 && i > 0) {
+ EVNode parentOfAncestor = parents.get(i - 1);
+ if (!parentOfAncestor.isQuery() && parentOfAncestor.dataSource instanceof JoinDataSource) {
+ crossedJoinLegBoundary = true;
+ }
+ }
+ if (ancestor.isQuery()) {
+ Query<?> q = ancestor.getQuery();
+ if (queriesSeen > 0) {
+ // To trust q's bound, every Query between the leaf and q must have been
+ // 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();
Review 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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org