Copilot commented on code in PR #17245: URL: https://github.com/apache/iotdb/pull/17245#discussion_r2929515505
########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/iterative/rule/PushPredicateThroughProjectIntoWindow.java: ########## @@ -0,0 +1,207 @@ +/* + * 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.iotdb.db.queryengine.plan.relational.planner.iterative.rule; + +import org.apache.iotdb.db.queryengine.plan.relational.planner.PlannerContext; +import org.apache.iotdb.db.queryengine.plan.relational.planner.Symbol; +import org.apache.iotdb.db.queryengine.plan.relational.planner.iterative.Rule; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.FilterNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.ProjectNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.TopKRankingNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.ValuesNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.WindowNode; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.ComparisonExpression; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.Expression; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.Literal; +import org.apache.iotdb.db.queryengine.plan.relational.sql.ast.SymbolReference; +import org.apache.iotdb.db.queryengine.plan.relational.utils.matching.Capture; +import org.apache.iotdb.db.queryengine.plan.relational.utils.matching.Captures; +import org.apache.iotdb.db.queryengine.plan.relational.utils.matching.Pattern; + +import com.google.common.collect.ImmutableList; + +import java.util.OptionalInt; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static java.lang.Math.toIntExact; +import static java.util.Objects.requireNonNull; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.iterative.rule.Util.toTopNRankingType; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.node.Patterns.filter; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.node.Patterns.project; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.node.Patterns.source; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.node.Patterns.window; +import static org.apache.iotdb.db.queryengine.plan.relational.utils.matching.Capture.newCapture; + +/** + * Converts a filter on a ranking function (e.g. {@code rn <= N}) into a {@link TopKRankingNode} + * when there is an identity projection between the filter and window node. + * + * <p>Before: + * + * <pre> + * FilterNode(rn <= N) + * └── ProjectNode (identity) + * └── WindowNode(row_number/rank) + * </pre> + * + * After (for LESS_THAN / LESS_THAN_OR_EQUAL — filter fully absorbed): + * + * <pre> + * ProjectNode (identity) + * └── TopKRankingNode(maxRanking=N) + * </pre> + * + * After (for EQUAL — filter kept): + * + * <pre> + * FilterNode(rn = N) + * └── ProjectNode (identity) + * └── TopKRankingNode(maxRanking=N) + * </pre> + */ +public class PushPredicateThroughProjectIntoWindow implements Rule<FilterNode> { + private static final Capture<ProjectNode> PROJECT = newCapture(); + private static final Capture<WindowNode> WINDOW = newCapture(); + + private final PlannerContext plannerContext; + private final Pattern<FilterNode> pattern; + + public PushPredicateThroughProjectIntoWindow(PlannerContext plannerContext) { + this.plannerContext = requireNonNull(plannerContext, "plannerContext is null"); Review Comment: `plannerContext` is stored in a field but never used in this rule, which will fail builds if unused fields are checked (and adds noise regardless). Remove the field and constructor assignment, or use it if it was intended for feature gating. ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/node/TopKRankingNode.java: ########## @@ -198,7 +223,8 @@ public int hashCode() { rankingType, rankingSymbol, maxRankingPerPartition, - partial); + partial, + dataPreSortedAndLimited); Review Comment: `hashCode()` now includes `dataPreSortedAndLimited`, but `equals()` (above) does not. This breaks the equals/hashCode contract and can cause incorrect behavior in hash-based collections or plan memoization. Make `equals()` and `hashCode()` consider the same set of fields (e.g., add `dataPreSortedAndLimited` to `equals()`). ########## integration-test/src/test/java/org/apache/iotdb/relational/it/db/it/IoTDBWindowFunction3IT.java: ########## @@ -166,6 +177,84 @@ public void testReplaceWindowWithRowNumber() { DATABASE_NAME); } + @Test + public void testPushDownFilterIntoWindowWithRank() { + // Data: d1 values {3,5,3,1}, d2 values {2,4} + // rank(PARTITION BY device ORDER BY value): + // d1: 1.0→rank=1, 3.0→rank=2, 3.0→rank=2, 5.0→rank=4 + // d2: 2.0→rank=1, 4.0→rank=2 + // WHERE rk <= 2: keeps d1 rows with rank≤2 (3 rows due to tie), d2 all (2 rows) + String[] expectedHeader = new String[] {"time", "device", "value", "rk"}; + String[] retArray = + new String[] { + "2021-01-01T09:05:00.000Z,d1,3.0,2,", + "2021-01-01T09:09:00.000Z,d1,3.0,2,", + "2021-01-01T09:10:00.000Z,d1,1.0,1,", + "2021-01-01T09:08:00.000Z,d2,2.0,1,", + "2021-01-01T09:15:00.000Z,d2,4.0,2,", + }; + tableResultSetEqualTest( + "SELECT * FROM (SELECT *, rank() OVER (PARTITION BY device ORDER BY value) as rk FROM demo) WHERE rk <= 2 ORDER BY device, time", + expectedHeader, + retArray, + DATABASE_NAME); + } + + @Test + public void testPushDownLimitIntoWindowWithRank() { + // TopKRanking(RANK, topN=2) keeps rank≤2 per partition, then LIMIT 2 on final result + // d1 rank≤2: 1.0(r=1), 3.0(r=2), 3.0(r=2) → 3 rows sorted by time: 09:05,09:09,09:10 + // d2 rank≤2: 2.0(r=1), 4.0(r=2) → 2 rows + // ORDER BY device, time LIMIT 2 → first 2 from d1 Review Comment: The test comment claims the query uses `TopKRanking(RANK, topN=2)` / rank<=2 semantics, but the SQL here has no `WHERE rk <= 2` predicate and therefore should not be described as a per-partition top-N optimization. Please update the comment to match the actual query semantics (or adjust the query if the intent was to test rank-filter pushdown). ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/distribute/TableDistributedPlanGenerator.java: ########## @@ -1894,12 +1965,24 @@ public List<PlanNode> visitTopKRanking(TopKRankingNode node, PlanContext context } List<PlanNode> childrenNodes = node.getChildren().get(0).accept(this, context); if (canSplitPushDown) { + // visitGroup may return GroupNode-wrapped children (sort not eliminated) or bare + // DeviceTableScanNode (sort eliminated). Unwrap GroupNode/SortNode when present. childrenNodes = childrenNodes.stream() - .map(child -> child.getChildren().get(0)) + .map(child -> child instanceof SortNode ? child.getChildren().get(0) : child) Review Comment: In `visitTopKRanking`, the code comment says it will unwrap `GroupNode/SortNode`, but the mapping only unwraps `SortNode`. `visitGroup()` actually wraps children in `GroupNode` when sort can't be eliminated, so `tryPushTopKRankingLimitToScan()` will never see `DeviceTableScanNode` in that case and the per-device limit pushdown won't happen. Consider unwrapping `GroupNode` here (and/or updating the comment) so the optimization works when children are GroupNode-wrapped. -- 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]
