924060929 commented on code in PR #70332:
URL: https://github.com/apache/doris/pull/70332#discussion_r4538708023
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/AbstractPhysicalPlan.java:
##########
@@ -62,6 +66,20 @@ public AbstractPhysicalPlan(PlanType type,
Optional<GroupExpression> groupExpres
physicalProperties == null ? PhysicalProperties.ANY :
physicalProperties;
}
+ /**
+ * Copy the node for profile display, sharing expressions and metadata but
detaching planner state.
+ * Capture output values: reusing LogicalProperties would retain its
suppliers and their plans.
+ * Operators with plan references outside children must also copy those
references.
+ */
+ public AbstractPhysicalPlan copyForProfile(Function<Plan, Plan> copyPlan) {
+ LogicalProperties properties = new
LogicalProperties(LazyCompute.ofInstance(getOutput()),
+ LazyCompute.ofInstance(getAsteriskOutput()), () ->
DataTrait.EMPTY_TRAIT);
+ List<Plan> copiedChildren =
children.stream().map(copyPlan).collect(Collectors.toList());
+ PhysicalPlan copy = (PhysicalPlan) withGroupExprLogicalPropChildren(
+ Optional.empty(), Optional.of(properties), copiedChildren);
Review Comment:
[P2] Ensure recursive CTE nodes actually use the copied children
`PhysicalRecursiveUnion`, `PhysicalRecursiveUnionAnchor`, and
`PhysicalRecursiveUnionProducer` currently ignore the `children` argument in
`withGroupExprLogicalPropChildren()` and rebuild with their original
`left()/right()/child()`. Calling that method here therefore leaves the profile
snapshot pointing into the original planner subtree, even though
`copiedChildren` was computed. This retains the original optimizer references
and lets `Profile.updateActualRowCountOnPhysicalPlan()` mutate planner-owned
Statistics below the recursive node. A focused test wrapping an OLAP scan in
`PhysicalRecursiveUnionProducer` confirms that
`PhysicalPlanShallowCopier.copy(producer).child(0)` is still the original scan.
Please fix these implementations to use the supplied children (or provide
equivalent profile-copy overrides), and cover recursive CTE nodes in the
isolation tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalPlanShallowCopier.java:
##########
@@ -0,0 +1,97 @@
+// 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.doris.nereids.trees.plans.physical;
+
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.util.MutableState;
+import org.apache.doris.statistics.model.Statistics;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Objects;
+
+/** Copies a physical plan for profile display without retaining Memo or
planner-owned mutable state. */
+public class PhysicalPlanShallowCopier {
+ private final Map<Plan, AbstractPhysicalPlan> copies = new
IdentityHashMap<>();
+ private final Map<RuntimeFilter, RuntimeFilter> filters = new
IdentityHashMap<>();
+
+ private PhysicalPlanShallowCopier() {
+ }
+
+ /** Copy a complete plan, then reconnect runtime filters exclusively to
copied nodes. */
+ public static PhysicalPlan copy(PhysicalPlan plan) {
+ PhysicalPlanShallowCopier copier = new PhysicalPlanShallowCopier();
+ return copier.copyPlan(plan);
+ }
+
+ private PhysicalPlan copyPlan(PhysicalPlan plan) {
+ AbstractPhysicalPlan result = copyNode(plan);
+ // RFs refer back to builder/target nodes. Rewire only after all plan
nodes have been copied.
+ for (Plan original : copies.keySet()) {
+ AbstractPhysicalPlan source = (AbstractPhysicalPlan) original;
+ source.getRuntimeFilters().forEach(this::copyFilter);
+ source.getAppliedRuntimeFilters().forEach(this::copyFilter);
+ }
+ // The RF constructor registers itself on its builder. Restore source
order independently
+ // of the order in which scans/builders were visited in the identity
map.
+ for (Map.Entry<Plan, AbstractPhysicalPlan> entry : copies.entrySet()) {
+ AbstractPhysicalPlan source = (AbstractPhysicalPlan)
entry.getKey();
+ AbstractPhysicalPlan copy = entry.getValue();
+ copy.getRuntimeFilters().clear();
+ source.getRuntimeFilters().forEach(filter ->
copy.addRuntimeFilter(filters.get(filter)));
+ source.getAppliedRuntimeFilters().forEach(filter ->
copy.addAppliedRuntimeFilter(filters.get(filter)));
+ }
+ return result;
+ }
+
+ private AbstractPhysicalPlan copyNode(Plan plan) {
+ AbstractPhysicalPlan existing = copies.get(plan);
+ if (existing != null) {
+ return existing;
+ }
+ AbstractPhysicalPlan source = (AbstractPhysicalPlan) plan;
+ AbstractPhysicalPlan copy = source.copyForProfile(this::copyNode);
+ copy.setMutableState(MutableState.KEY_GROUP,
source.getGroupIdAsString());
+ copy.setStatistics(source.getStats() == null ? null : new
Statistics(source.getStats()));
Review Comment:
[P2] Preserve actual-row display for lazy-materialized scans
For `PhysicalLazyMaterializeOlapScan` and `PhysicalLazyMaterializeFileScan`,
the constructor initially shares Statistics with the embedded scan, and
`toString()` prints that embedded scan rather than the wrapper's Statistics.
This assignment replaces the wrapper's Statistics with a second independent
copy. Profile updates actual rows only on nodes reached through `children()`,
while the embedded scan is a separate field, so its actual row count remains -1
and the physical-plan appendix stops displaying `actualRows` for these scans. I
reproduced this with an OLAP lazy scan: updating the original wrapper prints
`actualRows=42`; after copying, `copy.updateActualRowCount(84)` sets the
wrapper to 84 but `copy.treeString()` contains no `actualRows=84`. Please keep
updates and rendering on the same profile-owned Statistics, or explicitly
update/render the embedded node, and add an actual-row rendering test for these
wrappers.
--
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]
