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: [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]
