morrySnow commented on code in PR #65569:
URL: https://github.com/apache/doris/pull/65569#discussion_r3602094277
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/TopnFilterPushDownVisitor.java:
##########
@@ -263,7 +263,6 @@ public Boolean
visitPhysicalLazyMaterializeOlapScan(PhysicalLazyMaterializeOlapS
private boolean supportPhysicalRelations(PhysicalRelation relation) {
Review Comment:
PhysicalLazyMaterializeOlapScan was removed from supportPhysicalRelations().
This is correct because the new PlanVisitor now delegates
visitPhysicalLazyMaterializeOlapScan to visitPhysicalOlapScan, so the OlapScan
instanceof check covers it implicitly.
However, this dependency is non-obvious. A brief comment explaining why
PhysicalLazyMaterializeOlapScan no longer needs to be listed here would help
future maintainers.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/MaterializeSource.java:
##########
@@ -20,18 +20,46 @@
import org.apache.doris.nereids.trees.expressions.SlotReference;
import org.apache.doris.nereids.trees.plans.algebra.Relation;
-/**
- the table and slot used to do lazy materialize
- */
-public class MaterializeSource {
- public final Relation relation;
- public final SlotReference baseSlot;
-
- /*
- constructor
- */
+import java.util.Objects;
+
+/** Resolved physical source for one lazy output. */
+public final class MaterializeSource {
+ private final Relation relation;
Review Comment:
The relation and baseSlot fields changed from public final to private final
with getter methods. While this is a style improvement (better encapsulation),
it is a binary-incompatible API change. If any external code outside this PR
scope directly accesses .relation or .baseSlot on MaterializeSource objects, it
will fail to compile. Please verify there are no such references in other
PlanPostProcessor implementations or test utilities.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/materialize/TopNLazyMaterializationAnalyzer.java:
##########
@@ -0,0 +1,155 @@
+// 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.processor.post.materialize;
+
+import org.apache.doris.catalog.KeysType;
+import org.apache.doris.catalog.OlapTable;
+import
org.apache.doris.nereids.processor.post.materialize.MaterializationAnalysisResult.NotApplicableReason;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.SlotReference;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.physical.PhysicalCatalogRelation;
+import org.apache.doris.nereids.trees.plans.physical.PhysicalTopN;
+
+import com.google.common.collect.ImmutableSet;
+
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/** Orchestrates one lineage pass and one demand pass for a candidate physical
TopN. */
+public final class TopNLazyMaterializationAnalyzer {
+ private final SlotLineageAnalyzer lineageAnalyzer = new
SlotLineageAnalyzer();
+ private final SlotDemandAnalyzer demandAnalyzer = new SlotDemandAnalyzer();
+
+ /** Analyze one physical TopN and return either an immutable decision or a
structured skip reason. */
+ public MaterializationAnalysisResult<TopNLazyAnalysis> analyze(
+ PhysicalTopN<? extends Plan> topN, List<Slot> effectiveOutput) {
+ Map<Slot, MaterializeSource> candidates = new LinkedHashMap<>();
+ List<Slot> materializedSlots = new ArrayList<>();
+ Set<Slot> slotsToResolve = new LinkedHashSet<>(effectiveOutput);
+ collectInputSlots(topN, slotsToResolve);
+ Map<Slot, MaterializationAnalysisResult<OutputLineage>> lineages =
+ lineageAnalyzer.analyze(topN.child(),
ImmutableSet.copyOf(slotsToResolve).asList());
+
+ for (Slot outputSlot : effectiveOutput) {
+ if (!(outputSlot instanceof SlotReference)) {
+ materializedSlots.add(outputSlot);
+ continue;
+ }
+ MaterializationAnalysisResult<OutputLineage> lineageResult =
lineages.get(outputSlot);
+ if (!lineageResult.isApplicable() ||
!lineageResult.getValue().getSource().isPresent()) {
+ materializedSlots.add(outputSlot);
+ continue;
+ }
+ candidates.put(outputSlot,
lineageResult.getValue().getSource().get());
+ }
+
+ Set<Slot> requiredEagerSlots = new LinkedHashSet<>(
+ demandAnalyzer.analyzeRequiredEagerSlots(topN));
+ collectRequiredRelationSlots(topN, requiredEagerSlots);
+ Set<SourceColumnKey> eagerSourceColumns = new LinkedHashSet<>();
+ Set<Slot> eagerSlots = new LinkedHashSet<>(requiredEagerSlots);
+ eagerSlots.addAll(materializedSlots);
+ for (Slot eagerSlot : eagerSlots) {
+ MaterializationAnalysisResult<OutputLineage> lineage =
lineages.get(eagerSlot);
+ if (lineage != null && lineage.isApplicable() &&
lineage.getValue().getSource().isPresent()) {
+
eagerSourceColumns.add(lineage.getValue().getSource().get().getSourceColumnKey());
+ }
+ }
+ removeConflictingCandidates(candidates, requiredEagerSlots,
+ ImmutableSet.copyOf(materializedSlots), eagerSourceColumns);
+ for (Slot outputSlot : effectiveOutput) {
+ if (!candidates.containsKey(outputSlot) &&
!materializedSlots.contains(outputSlot)) {
+ materializedSlots.add(outputSlot);
+ }
+ }
+ if (candidates.isEmpty()) {
+ return MaterializationAnalysisResult.notApplicable(
+ NotApplicableReason.NO_DEFERRED_OUTPUT, topN.shapeInfo());
+ }
+
+ return MaterializationAnalysisResult.applicable(
+ new TopNLazyAnalysis(materializedSlots, candidates));
+ }
+
+ static void removeConflictingCandidates(Map<Slot, MaterializeSource>
candidates,
+ Set<Slot> requiredEagerSlots, Set<Slot> materializedSlots) {
+ removeConflictingCandidates(candidates, requiredEagerSlots,
materializedSlots, new LinkedHashSet<>());
+ }
+
+ private static void removeConflictingCandidates(Map<Slot,
MaterializeSource> candidates,
+ Set<Slot> requiredEagerSlots, Set<Slot> materializedSlots,
+ Set<SourceColumnKey> eagerSourceColumns) {
+ // Fetch is scheduled per physical source column. If any alias of a
column is needed eagerly,
+ // every output backed by the same column must remain eager as well.
+ for (Map.Entry<Slot, MaterializeSource> entry : candidates.entrySet())
{
+ if (requiredEagerSlots.contains(entry.getKey())
+ ||
requiredEagerSlots.contains(entry.getValue().getBaseSlot())
+ || materializedSlots.contains(entry.getKey())
+ ||
materializedSlots.contains(entry.getValue().getBaseSlot())) {
+ eagerSourceColumns.add(entry.getValue().getSourceColumnKey());
+ }
+ }
+ candidates.entrySet().removeIf(
+ entry ->
eagerSourceColumns.contains(entry.getValue().getSourceColumnKey()));
+ }
+
+ private void collectInputSlots(Plan plan, Set<Slot> slots) {
+ slots.addAll(plan.getInputSlots());
+ for (Plan child : plan.children()) {
+ collectInputSlots(child, slots);
+ }
+ }
+
+ private void collectRequiredRelationSlots(Plan plan, Set<Slot>
requiredSlots) {
+ if (plan instanceof PhysicalCatalogRelation) {
+ collectRequiredRelationSlots((PhysicalCatalogRelation) plan,
requiredSlots);
+ }
+ for (Plan child : plan.children()) {
+ collectRequiredRelationSlots(child, requiredSlots);
+ }
+ }
+
+ private void collectRequiredRelationSlots(PhysicalCatalogRelation
relation, Set<Slot> requiredSlots) {
+ if (relation.getTable() instanceof OlapTable) {
+ OlapTable table = (OlapTable) relation.getTable();
+ if (KeysType.UNIQUE_KEYS.equals(table.getKeysType())
Review Comment:
The condition mixing && and || operators is correct due to Java operator
precedence (&& binds tighter than ||), but is easy to misread:
```java
if (KeysType.UNIQUE_KEYS.equals(table.getKeysType())
&& !table.getTableProperty().getEnableUniqueKeyMergeOnWrite()
|| KeysType.AGG_KEYS.equals(table.getKeysType())
|| KeysType.PRIMARY_KEYS.equals(table.getKeysType()))
```
This evaluates as: (UNIQUE_KEYS && !MoW) || AGG_KEYS || PRIMARY_KEYS.
Consider adding explicit parentheses to make the intent immediately clear,
since this logic was moved from its original location.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalLazyMaterializeTVFScan.java:
##########
@@ -32,41 +32,42 @@
import java.util.List;
import java.util.Optional;
-/**
- wrapper for PhysicalTVFRelation used for lazy materialization
- */
+/** TVF scan wrapper that replaces deferred columns with a row-id used by the
materialize node. */
public class PhysicalLazyMaterializeTVFScan extends PhysicalTVFRelation {
- private PhysicalTVFRelation scan;
- private SlotReference rowId;
+ private final PhysicalTVFRelation scan;
+ private final SlotReference rowId;
private final List<Slot> lazySlots;
- private List<Slot> output;
- /**
- * PhysicalLazyMaterializeTVFScan
- */
public PhysicalLazyMaterializeTVFScan(PhysicalTVFRelation scan,
SlotReference rowId, List<Slot> lazySlots) {
- super(scan.getRelationId(), scan.getFunction(),
scan.getOperativeSlots(), scan.getLogicalProperties());
+ this(scan, rowId, lazySlots, scan.getGroupExpression(), null,
+ scan.getPhysicalProperties(), scan.getStats());
+ }
+
+ private PhysicalLazyMaterializeTVFScan(PhysicalTVFRelation scan,
SlotReference rowId, List<Slot> lazySlots,
+ Optional<GroupExpression> groupExpression, LogicalProperties
logicalProperties,
+ PhysicalProperties physicalProperties, Statistics statistics) {
+ super(scan.getRelationId(), scan.getFunction(),
scan.getOperativeSlots(),
+ groupExpression, logicalProperties, physicalProperties,
statistics);
this.scan = scan;
this.rowId = rowId;
- this.lazySlots = lazySlots;
+ this.lazySlots = ImmutableList.copyOf(lazySlots);
}
@Override
public List<Slot> computeOutput() {
Review Comment:
The old code cached computeOutput() with an `output == null` guard. The new
code recomputes the output on every call. While the result is stable (derived
from scan.getOutput() minus lazy slots plus row ID), every getOutput() or
getOutputSet() call in the rewriter loop will re-allocate the output list.
For wide TVF scans with many lazy slots, this could add measurable overhead.
Consider restoring the cached output pattern, or ensuring callers cache the
result locally.
--
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]
