Copilot commented on code in PR #17208:
URL: https://github.com/apache/iotdb/pull/17208#discussion_r2850469654
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/distribute/TableDistributedPlanGenerator.java:
##########
@@ -1914,6 +1915,36 @@ public List<PlanNode> visitTopKRanking(TopKRankingNode
node, PlanContext context
}
}
+ @Override
+ public List<PlanNode> visitLimitKRanking(LimitKRankingNode node, PlanContext
context) {
+ Optional<OrderingScheme> orderingScheme =
node.getSpecification().getOrderingScheme();
+ if (orderingScheme.isPresent()) {
+ context.setExpectedOrderingScheme(orderingScheme.get());
+ nodeOrderingMap.put(node.getPlanNodeId(), orderingScheme.get());
+ }
+
+ checkArgument(
+ node.getChildren().size() == 1, "Size of LimitKRankingNode can only be
1 in logical plan.");
+ boolean canSplitPushDown = node.getChild() instanceof GroupNode;
+ if (!canSplitPushDown) {
+ node.setChild(((SortNode) node.getChild()).getChild());
+ }
+ List<PlanNode> childrenNodes = node.getChildren().get(0).accept(this,
context);
Review Comment:
The visitLimitKRanking method is missing the child unwrapping logic that is
present in visitTopKRanking. When canSplitPushDown is true, the
visitTopKRanking method unwraps GroupNode children (lines 1897-1902), but this
logic is absent here. This could lead to incorrect plan structure when
LimitKRankingNode has a GroupNode child and there are multiple child nodes
after distribution.
```suggestion
List<PlanNode> childrenNodes = node.getChildren().get(0).accept(this,
context);
if (canSplitPushDown) {
childrenNodes =
childrenNodes.stream()
.map(child -> child.getChildren().get(0))
.collect(Collectors.toList());
}
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/iterative/rule/Util.java:
##########
@@ -142,4 +146,34 @@ public static Optional<TopKRankingNode.RankingType>
toTopNRankingType(WindowNode
}
return Optional.empty();
}
+
+ /**
+ * Returns true when the window is ROW_NUMBER with ORDER BY on a single
TIMESTAMP column (ASC or
+ * DESC). In IoTDB, data is naturally sorted by (device, time) and table
scans support both
+ * forward and reverse iteration, so we can use the streaming
LimitKRankingNode instead of the
+ * buffered TopKRankingNode for both ASC (earliest K) and DESC (latest K).
Review Comment:
The method documentation states "Returns true when the window is ROW_NUMBER"
but the implementation also accepts RANK (line 160). The documentation should
be updated to mention that both ROW_NUMBER and RANK are supported.
```suggestion
* Returns true when the window is ROW_NUMBER or RANK with ORDER BY on a
single TIMESTAMP column
* (ASC or DESC). In IoTDB, data is naturally sorted by (device, time) and
table scans support
* both forward and reverse iteration, so we can use the streaming
LimitKRankingNode instead of
* the buffered TopKRankingNode for both ASC (earliest K) and DESC (latest
K).
```
--
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]
