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