github-actions[bot] commented on code in PR #61343:
URL: https://github.com/apache/doris/pull/61343#discussion_r2936340370
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalDeferMaterializeTopN.java:
##########
@@ -122,25 +123,28 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C
context) {
@Override
public Plan withGroupExpression(Optional<GroupExpression> groupExpression)
{
- return new LogicalDeferMaterializeTopN<>(logicalTopN,
deferMaterializeSlotIds, columnIdSlot,
- groupExpression, Optional.of(getLogicalProperties()), child());
+ return AbstractPlan.copyWithSameId(this, () ->
+ new LogicalDeferMaterializeTopN<>(logicalTopN,
deferMaterializeSlotIds, columnIdSlot,
+ groupExpression, Optional.of(getLogicalProperties()),
child()));
}
@Override
public Plan withGroupExprLogicalPropChildren(Optional<GroupExpression>
groupExpression,
Optional<LogicalProperties> logicalProperties, List<Plan>
children) {
Preconditions.checkArgument(children.size() == 1,
"LogicalDeferMaterializeTopN should have 1 child, but input is
%s", children.size());
- return new
LogicalDeferMaterializeTopN<>(logicalTopN.withChildren(ImmutableList.of(children.get(0))),
- deferMaterializeSlotIds, columnIdSlot, groupExpression,
logicalProperties, children.get(0));
+ return AbstractPlan.copyWithSameId(this, () ->
+ new
LogicalDeferMaterializeTopN<>(logicalTopN.withChildren(ImmutableList.of(children.get(0))),
+ deferMaterializeSlotIds, columnIdSlot, groupExpression,
logicalProperties, children.get(0)));
Review Comment:
**Bug: Nested `copyWithSameId` breaks outer ID inheritance.**
`logicalTopN.withChildren(...)` is called *inside* the `copyWithSameId`
lambda. Since `LogicalTopN.withChildren` also calls `copyWithSameId`, the
execution flow is:
1. Outer `copyWithSameId` sets `INHERIT_ID = this.id` (the
DeferMaterializeTopN's ID)
2. Lambda starts executing, encounters `logicalTopN.withChildren(...)`
3. Inner `copyWithSameId` **overwrites** `INHERIT_ID = logicalTopN.id`
4. Inner `LogicalTopN` constructor consumes and clears `INHERIT_ID`
5. Inner `copyWithSameId` finally block calls `INHERIT_ID.remove()`
6. Now lambda calls `new LogicalDeferMaterializeTopN(...)` — but
`INHERIT_ID` is `null`
7. Outer constructor allocates a **fresh ID** instead of inheriting
**Fix:** Hoist the inner call before `copyWithSameId`:
```java
LogicalTopN<?> newTopN =
logicalTopN.withChildren(ImmutableList.of(children.get(0)));
return AbstractPlan.copyWithSameId(this, () ->
new LogicalDeferMaterializeTopN<>(newTopN, ...));
```
Same issue applies to `withChildren` below.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalDeferMaterializeResultSink.java:
##########
@@ -78,15 +79,17 @@ public long getSelectedIndexId() {
public LogicalDeferMaterializeResultSink<Plan> withChildren(List<Plan>
children) {
Preconditions.checkArgument(children.size() == 1,
"LogicalDeferMaterializeResultSink only accepts one child");
- return new LogicalDeferMaterializeResultSink<>(
+ return AbstractPlan.copyWithSameId(this, () ->
+ new LogicalDeferMaterializeResultSink<>(
logicalResultSink.withChildren(ImmutableList.of(children.get(0))),
- olapTable, selectedIndexId, Optional.empty(),
Optional.empty(), children.get(0));
Review Comment:
**Bug: Nested `copyWithSameId` — same issue as
`LogicalDeferMaterializeTopN`.**
`logicalResultSink.withChildren(...)` inside the `copyWithSameId` lambda
will overwrite `INHERIT_ID`, causing the outer
`LogicalDeferMaterializeResultSink` to get a fresh ID instead of preserving
`this.id`.
**Fix:** Hoist `logicalResultSink.withChildren(...)` before the
`copyWithSameId` call. Same fix needed for `withGroupExprLogicalPropChildren`
below.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalDeferMaterializeResultSink.java:
##########
@@ -82,10 +83,10 @@ public long getSelectedIndexId() {
public Plan withChildren(List<Plan> children) {
Preconditions.checkArgument(children.size() == 1,
"PhysicalDeferMaterializeResultSink's children size must be 1,
but real is %s", children.size());
- return new PhysicalDeferMaterializeResultSink<>(
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalDeferMaterializeResultSink<>(
physicalResultSink.withChildren(ImmutableList.of(children.get(0))),
olapTable, selectedIndexId, groupExpression,
getLogicalProperties(),
Review Comment:
**Bug: Nested `copyWithSameId` — same issue.**
`physicalResultSink.withChildren(...)` inside the lambda overwrites
`INHERIT_ID`. Hoist the inner call before `copyWithSameId`. Same fix needed for
`withGroupExprLogicalPropChildren`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalHashJoin.java:
##########
@@ -173,25 +174,25 @@ public PhysicalHashJoin<Plan, Plan>
withChildren(List<Plan> children) {
@Override
public PhysicalHashJoin<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE>
withGroupExpression(
Optional<GroupExpression> groupExpression) {
- return new PhysicalHashJoin<>(joinType, hashJoinConjuncts,
otherJoinConjuncts,
- markJoinConjuncts, hint, markJoinSlotReference,
groupExpression,
- getLogicalProperties(), null, null, left(), right());
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalHashJoin<>(joinType, hashJoinConjuncts,
+ otherJoinConjuncts, markJoinConjuncts, hint,
markJoinSlotReference, groupExpression,
Review Comment:
**Bug: `withChildren()` (line 162-172) is not wrapped with
`copyWithSameId`**, while all three sibling methods (`withGroupExpression`,
`withGroupExprLogicalPropChildren`, `withPhysicalPropertiesAndStats`) are
wrapped. This means `PhysicalHashJoin.withChildren()` will allocate a fresh ID,
breaking the invariant this PR establishes.
The multi-statement pattern (create, conditionally set mutable state,
return) can still use `copyWithSameId` — apply it to just the constructor call
and then set mutable state on the returned object:
```java
public PhysicalHashJoin<Plan, Plan> withChildren(List<Plan> children) {
Preconditions.checkArgument(children.size() == 2);
PhysicalHashJoin newJoin = AbstractPlan.copyWithSameId(this, () ->
new PhysicalHashJoin<>(joinType, hashJoinConjuncts, ...));
if (groupExpression.isPresent()) {
newJoin.setMutableState(MutableState.KEY_GROUP, ...);
}
return newJoin;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalNestedLoopJoin.java:
##########
@@ -174,26 +175,26 @@ public PhysicalNestedLoopJoin<Plan, Plan>
withChildren(List<Plan> children) {
@Override
public PhysicalNestedLoopJoin<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE>
withGroupExpression(
Optional<GroupExpression> groupExpression) {
- return new PhysicalNestedLoopJoin<>(joinType,
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalNestedLoopJoin<>(joinType,
hashJoinConjuncts, otherJoinConjuncts, markJoinConjuncts,
markJoinSlotReference,
Review Comment:
**Bug: `withChildren()` (line 164-173) is not wrapped with
`copyWithSameId`**, same issue as `PhysicalHashJoin`. The `setMutableState`
pattern can coexist with `copyWithSameId` — wrap just the constructor call.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalDeferMaterializeTopN.java:
##########
@@ -94,17 +95,19 @@ public long getLimit() {
}
public PhysicalDeferMaterializeTopN<? extends Plan>
withPhysicalTopN(PhysicalTopN<? extends Plan> physicalTopN) {
- return new PhysicalDeferMaterializeTopN<>(physicalTopN,
deferMaterializeSlotIds, columnIdSlot, groupExpression,
- getLogicalProperties(), physicalProperties, statistics,
physicalTopN.child());
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalDeferMaterializeTopN<>(physicalTopN,
+ deferMaterializeSlotIds, columnIdSlot, groupExpression,
+ getLogicalProperties(), physicalProperties, statistics,
physicalTopN.child()));
}
@Override
public PhysicalDeferMaterializeTopN<? extends Plan>
withChildren(List<Plan> children) {
Preconditions.checkArgument(children.size() == 1,
"PhysicalDeferMaterializeTopN's children size must be 1, but
real is %s", children.size());
- return new
PhysicalDeferMaterializeTopN<>(physicalTopN.withChildren(ImmutableList.of(children.get(0))),
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalDeferMaterializeTopN<>(
+ physicalTopN.withChildren(ImmutableList.of(children.get(0))),
deferMaterializeSlotIds, columnIdSlot, groupExpression,
getLogicalProperties(),
Review Comment:
**Bug: Nested `copyWithSameId` — same issue.**
`physicalTopN.withChildren(...)` inside the lambda overwrites `INHERIT_ID`,
causing the outer `PhysicalDeferMaterializeTopN` to get a fresh ID. Hoist the
inner call before `copyWithSameId`. Same fix needed for
`withGroupExprLogicalPropChildren`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalStorageLayerAggregate.java:
##########
@@ -82,29 +83,29 @@ public String toString() {
}
public PhysicalStorageLayerAggregate withPhysicalOlapScan(PhysicalOlapScan
physicalOlapScan) {
- return new PhysicalStorageLayerAggregate(physicalOlapScan, aggOp);
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalStorageLayerAggregate(physicalOlapScan, aggOp));
}
@Override
public PhysicalStorageLayerAggregate
withGroupExpression(Optional<GroupExpression> groupExpression) {
- return new PhysicalStorageLayerAggregate(relation, aggOp,
groupExpression,
- getLogicalProperties(), physicalProperties, statistics);
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalStorageLayerAggregate(relation, aggOp,
+ groupExpression, getLogicalProperties(), physicalProperties,
statistics));
}
@Override
public Plan withGroupExprLogicalPropChildren(Optional<GroupExpression>
groupExpression,
Optional<LogicalProperties> logicalProperties, List<Plan>
children) {
- return new PhysicalStorageLayerAggregate(relation, aggOp,
groupExpression,
- logicalProperties.get(), physicalProperties, statistics);
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalStorageLayerAggregate(relation, aggOp,
+ groupExpression, logicalProperties.get(), physicalProperties,
statistics));
}
@Override
public PhysicalPlan withPhysicalPropertiesAndStats(PhysicalProperties
physicalProperties,
Statistics statistics) {
- return new PhysicalStorageLayerAggregate(
+ return AbstractPlan.copyWithSameId(this, () -> new
PhysicalStorageLayerAggregate(
(PhysicalCatalogRelation)
relation.withPhysicalPropertiesAndStats(null, statistics),
aggOp, groupExpression,
Review Comment:
**Bug: Nested `copyWithSameId`.**
`relation.withPhysicalPropertiesAndStats(null, statistics)` is called inside
the lambda. Since `PhysicalOlapScan.withPhysicalPropertiesAndStats` also uses
`copyWithSameId`, this will overwrite `INHERIT_ID` and the outer
`PhysicalStorageLayerAggregate` will get a fresh ID.
**Fix:**
```java
PhysicalCatalogRelation newRelation = (PhysicalCatalogRelation)
relation.withPhysicalPropertiesAndStats(null, statistics);
return AbstractPlan.copyWithSameId(this, () -> new
PhysicalStorageLayerAggregate(
newRelation, aggOp, groupExpression,
getLogicalProperties(), physicalProperties, statistics));
```
--
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]
