Copilot commented on code in PR #57840:
URL: https://github.com/apache/doris/pull/57840#discussion_r2522703790
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/copier/LogicalPlanDeepCopier.java:
##########
@@ -186,7 +186,16 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, De
List<NamedExpression> outputExpressions =
aggregate.getOutputExpressions().stream()
.map(o -> (NamedExpression)
ExpressionDeepCopier.INSTANCE.deepCopy(o, context))
.collect(ImmutableList.toImmutableList());
- return aggregate.withChildGroupByAndOutput(groupByExpressions,
outputExpressions, child);
+ LogicalAggregate<Plan> copiedAggregate =
aggregate.withChildGroupByAndOutput(groupByExpressions,
+ outputExpressions, child);
+ if (copiedAggregate.getSourceRepeat().isPresent()) {
+ Optional<LogicalRepeat<? extends Plan>> childRepeat =
+
copiedAggregate.collectFirst(LogicalRepeat.class::isInstance);
+ if (childRepeat.isPresent()) {
+ copiedAggregate =
copiedAggregate.withSourceRepeat(childRepeat.get());
Review Comment:
The use of `collectFirst` to find the source repeat may be unreliable in
complex plan trees. The `collectFirst` method performs a depth-first search and
returns the first `LogicalRepeat` it encounters in the tree.
This could be problematic if:
1. There are nested aggregates with repeats in the subtree
2. The plan has been transformed and the repeat is no longer a direct child
but is nested deeper
Consider directly checking if the child is a `LogicalRepeat` instance
instead:
```java
if (copiedAggregate.getSourceRepeat().isPresent()) {
if (child instanceof LogicalRepeat) {
copiedAggregate = copiedAggregate.withSourceRepeat((LogicalRepeat<?
extends Plan>) child);
}
}
```
This ensures we're referencing the immediate child repeat node, which is the
expected semantic for `sourceRepeat` based on its usage in other parts of the
codebase (e.g., `PushDownFilterThroughAggregation` assumes sourceRepeat refers
to the aggregate's direct relationship with a repeat operation).
```suggestion
if (child instanceof LogicalRepeat) {
copiedAggregate =
copiedAggregate.withSourceRepeat((LogicalRepeat<? extends Plan>) child);
```
--
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]
