morrySnow commented on code in PR #63899:
URL: https://github.com/apache/doris/pull/63899#discussion_r3550265931
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -743,6 +762,64 @@ public Plan getOriginalPlan() {
return originalPlan;
}
+ private Expression shuttleExpressionWithLineage(Expression expression,
Plan plan) {
+ return shuttleExpressionWithLineage(ImmutableList.of(expression),
plan).get(0);
+ }
+
+ private List<? extends Expression> shuttleExpressionWithLineage(List<?
extends Expression> expressions,
+ Plan plan) {
+ if (expressions.isEmpty()) {
+ return ImmutableList.of();
+ }
+ ExpressionLineageReplacer.ExpressionReplaceContext replaceContext =
+ new
ExpressionLineageReplacer.ExpressionReplaceContext(expressions);
+ for (NamedExpression namedExpression :
getLineageExpressionIndex(plan)) {
+ if
(!replaceContext.getUsedExprIdSet().contains(namedExpression.getExprId())) {
+ continue;
+ }
+
namedExpression.accept(ExpressionLineageReplacer.NamedExpressionCollector.INSTANCE,
replaceContext);
+ }
+ List<? extends Expression> replacedExpressions =
replaceContext.getReplacedExpressions();
+ if (replacedExpressions == null || expressions.size() !=
replacedExpressions.size()) {
Review Comment:
This fallback check is effectively dead code: `getReplacedExpressions()`
always returns a non-null list (computed via `Collectors.toList()`) with the
same size as the input (1:1 stream `map`). The `replacedExpressions == null`
branch is unreachable, and the size mismatch can never occur. The fallback to
`ExpressionUtils.shuttleExpressionWithLineage` provides a false sense of safety
— if the index-based approach silently produces incorrect results with the
correct count, this check won't catch it. Consider either removing the fallback
(if the index approach is trusted) or adding a more meaningful validation
(e.g., comparing results against the original method in a debug assertion).
Reference: `ExpressionReplaceContext.getReplacedExpressions()` at
ExpressionLineageReplacer.java:180-187 always returns a non-null list of size
equal to `targetExpressions.size()`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -743,6 +762,64 @@ public Plan getOriginalPlan() {
return originalPlan;
}
+ private Expression shuttleExpressionWithLineage(Expression expression,
Plan plan) {
+ return shuttleExpressionWithLineage(ImmutableList.of(expression),
plan).get(0);
+ }
+
+ private List<? extends Expression> shuttleExpressionWithLineage(List<?
extends Expression> expressions,
+ Plan plan) {
+ if (expressions.isEmpty()) {
+ return ImmutableList.of();
+ }
+ ExpressionLineageReplacer.ExpressionReplaceContext replaceContext =
+ new
ExpressionLineageReplacer.ExpressionReplaceContext(expressions);
+ for (NamedExpression namedExpression :
getLineageExpressionIndex(plan)) {
+ if
(!replaceContext.getUsedExprIdSet().contains(namedExpression.getExprId())) {
+ continue;
+ }
+
namedExpression.accept(ExpressionLineageReplacer.NamedExpressionCollector.INSTANCE,
replaceContext);
+ }
+ List<? extends Expression> replacedExpressions =
replaceContext.getReplacedExpressions();
+ if (replacedExpressions == null || expressions.size() !=
replacedExpressions.size()) {
+ return
ExpressionUtils.shuttleExpressionWithLineage(expressions, plan);
+ }
+ return replacedExpressions;
+ }
+
+ private List<? extends Expression>
shuttleAndNormalizeExpressionWithLineage(
+ Collection<? extends Expression> expressions, Plan plan) {
+ if (expressions.isEmpty()) {
+ return ImmutableList.of();
+ }
+ List<? extends Expression> shuttledExpressions =
+
shuttleExpressionWithLineage(ImmutableList.copyOf(expressions), plan);
+ List<Expression> normalizedExpressions = new
ArrayList<>(shuttledExpressions.size());
+ for (Expression expression : shuttledExpressions) {
+ normalizedExpressions.add(normalizeExpression(expression));
+ }
+ return normalizedExpressions;
+ }
+
+ private Expression normalizeExpression(Expression expression) {
+ Expression normalizedExpression =
normalizedExpressionMap.get(expression);
+ if (normalizedExpression == null) {
+ normalizedExpression =
expressionNormalization.rewrite(expression, expressionRewriteContext);
+ normalizedExpressionMap.put(expression, normalizedExpression);
+ }
+ return normalizedExpression;
+ }
+
+ private List<NamedExpression> getLineageExpressionIndex(Plan plan) {
+ List<NamedExpression> lineageExpressionIndex =
planLineageExpressionIndexes.get(plan);
+ if (lineageExpressionIndex == null) {
+ List<NamedExpression> collectedIndex = new ArrayList<>();
+ plan.accept(LineageExpressionCollector.INSTANCE,
collectedIndex);
+ lineageExpressionIndex = ImmutableList.copyOf(collectedIndex);
Review Comment:
`ImmutableList.copyOf(collectedIndex)` creates an O(n) defensive copy of a
locally-created `ArrayList` that is never exposed for mutation. The
`collectedIndex` is allocated, populated by the visitor, and immediately
wrapped — no external code can hold a reference to it. Returning
`collectedIndex` directly (or using `Collections.unmodifiableList()`) would
avoid the unnecessary allocation. This is called once per unique Plan object
visited during the check.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -743,6 +762,64 @@ public Plan getOriginalPlan() {
return originalPlan;
}
+ private Expression shuttleExpressionWithLineage(Expression expression,
Plan plan) {
+ return shuttleExpressionWithLineage(ImmutableList.of(expression),
plan).get(0);
+ }
+
+ private List<? extends Expression> shuttleExpressionWithLineage(List<?
extends Expression> expressions,
+ Plan plan) {
+ if (expressions.isEmpty()) {
+ return ImmutableList.of();
+ }
+ ExpressionLineageReplacer.ExpressionReplaceContext replaceContext =
+ new
ExpressionLineageReplacer.ExpressionReplaceContext(expressions);
+ for (NamedExpression namedExpression :
getLineageExpressionIndex(plan)) {
+ if
(!replaceContext.getUsedExprIdSet().contains(namedExpression.getExprId())) {
+ continue;
+ }
+
namedExpression.accept(ExpressionLineageReplacer.NamedExpressionCollector.INSTANCE,
replaceContext);
+ }
+ List<? extends Expression> replacedExpressions =
replaceContext.getReplacedExpressions();
+ if (replacedExpressions == null || expressions.size() !=
replacedExpressions.size()) {
+ return
ExpressionUtils.shuttleExpressionWithLineage(expressions, plan);
+ }
+ return replacedExpressions;
+ }
+
+ private List<? extends Expression>
shuttleAndNormalizeExpressionWithLineage(
+ Collection<? extends Expression> expressions, Plan plan) {
+ if (expressions.isEmpty()) {
+ return ImmutableList.of();
+ }
+ List<? extends Expression> shuttledExpressions =
+
shuttleExpressionWithLineage(ImmutableList.copyOf(expressions), plan);
Review Comment:
`ImmutableList.copyOf(expressions)` in
`shuttleAndNormalizeExpressionWithLineage` creates a defensive copy of the
input collection. However, the called `shuttleExpressionWithLineage` stores the
list only in a local `ExpressionReplaceContext` which reads it via iteration
(never mutates). For the `partitionExpressions` call site (a
locally-constructed `ArrayList`), this copy is entirely redundant. The only
benefit is stabilizing iteration order for `HashSet` inputs at the
`expressionsToCheck` call site — consider using `new ArrayList<>(expressions)`
instead to make the intent (listification, not defensive copying) clear.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -683,8 +690,19 @@ public static final class PartitionIncrementCheckContext {
private final Set<Set<Slot>> shuttledEqualSlotSet = new HashSet<>();
private final Map<CTEId, Plan> producerCteIdToPlanMap;
private final Plan originalPlan;
+ // Cache lineage-visible named expressions per plan identity to avoid
repeated full plan walks.
+ private final Map<Plan, List<NamedExpression>>
planLineageExpressionIndexes = new IdentityHashMap<>();
Review Comment:
The lineage expression index (`planLineageExpressionIndexes`, keyed by Plan
identity) is scoped to a single `PartitionIncrementCheckContext`. However,
`PartitionIncrementChecker` creates child contexts for each union-all branch —
so the same plan sub-tree passed to multiple child contexts will have its index
rebuilt from scratch each time. Plans in Nereids already use `LazyCompute` for
derived properties (e.g., `AbstractPlan.logicalPropertiesSupplier`). Consider
making this index a lazily-computed property on the Plan itself, which would
provide unlimited cache scope with the same lazy-computation semantics.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -772,6 +849,25 @@ public Void
visitLogicalCatalogRelation(LogicalCatalogRelation relation,
}
}
+ private static final class LineageExpressionCollector extends
DefaultPlanVisitor<Void, List<NamedExpression>> {
Review Comment:
`LineageExpressionCollector` duplicates the plan-walking pattern (singleton
`DefaultPlanVisitor`, iterate `plan.getExpressions()`, recurse via
`super.visit()`) already implemented in at least three other places:
- `NondeterministicFunctionCollector` at
`nereids/trees/plans/visitor/NondeterministicFunctionCollector.java`
- `ExpressionLineageReplacer.visit()` at
`nereids/trees/plans/visitor/ExpressionLineageReplacer.java:52-66`
- `StructInfo.RelationCollector` at
`nereids/rules/exploration/mv/StructInfo.java`
Consider extracting a shared utility (e.g.,
`PlanUtils.collectNamedExpressions(plan)`) to avoid maintaining the same logic
in four places.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -683,8 +690,19 @@ public static final class PartitionIncrementCheckContext {
private final Set<Set<Slot>> shuttledEqualSlotSet = new HashSet<>();
private final Map<CTEId, Plan> producerCteIdToPlanMap;
private final Plan originalPlan;
+ // Cache lineage-visible named expressions per plan identity to avoid
repeated full plan walks.
+ private final Map<Plan, List<NamedExpression>>
planLineageExpressionIndexes = new IdentityHashMap<>();
+ // Cache normalized expressions within this check context;
normalization uses the same CascadesContext.
+ private final Map<Expression, Expression> normalizedExpressionMap =
new IdentityHashMap<>();
Review Comment:
`normalizedExpressionMap` uses `IdentityHashMap<Expression, Expression>`,
which compares keys by reference equality (`==`). Since
`shuttleExpressionWithLineage` produces newly-created expression objects each
time (via `ExpressionReplacer`), the identity-based cache will almost never
hit. Each lookup adds HashMap overhead without meaningful benefit. Consider
either:
1. Removing this cache entirely if it doesn't provide measurable benefit in
profiling, or
2. Using a `HashMap` with proper `equals()` semantics (though structural
equality may still not help much with fresh shuttle results).
The `ExpressionNormalization.rewrite()` is already quite fast for the
typical `DateTrunc`/`Literal`/`SlotReference` expressions encountered here.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -259,29 +261,41 @@ public Void visitLogicalJoin(LogicalJoin<? extends Plan,
? extends Plan> join,
// check join type and partition column side
Set<Slot> leftColumnSet = join.child(0).getOutputSet();
Set<NamedExpression> namedExpressions = new
HashSet<>(context.getPartitionAndRefExpressionMap().keySet());
+ boolean needVisitJoinChildren = false;
+ boolean needCollectInvalidRight = false;
Review Comment:
Three boolean flags (`needVisitJoinChildren`, `needCollectInvalidRight`,
`needCollectInvalidLeft`) encode implicit coupling in `visitLogicalJoin`. Since
`context.collectInvalidTableSet()` is idempotent (it adds to a
`Set<LogicalCatalogRelation>`), the deferred execution of the two
`collectInvalidTableSet` calls is unnecessary complexity. Only `visit(join,
context)` genuinely benefits from deferral (it was previously called N times in
the loop). Consider simplifying to a single `needVisitJoinChildren` flag, and
move the `collectInvalidTableSet` calls back inside the loop where they are
contextually meaningful alongside the join-type check.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -683,8 +690,19 @@ public static final class PartitionIncrementCheckContext {
private final Set<Set<Slot>> shuttledEqualSlotSet = new HashSet<>();
private final Map<CTEId, Plan> producerCteIdToPlanMap;
private final Plan originalPlan;
+ // Cache lineage-visible named expressions per plan identity to avoid
repeated full plan walks.
+ private final Map<Plan, List<NamedExpression>>
planLineageExpressionIndexes = new IdentityHashMap<>();
+ // Cache normalized expressions within this check context;
normalization uses the same CascadesContext.
+ private final Map<Expression, Expression> normalizedExpressionMap =
new IdentityHashMap<>();
+ // Reuse the normalization rewriter during one partition lineage check.
+ private final ExpressionNormalization expressionNormalization = new
ExpressionNormalization();
Review Comment:
`ExpressionNormalization` is fully stateless — it wraps an
`ExpressionRuleExecutor` with a `final` list of 21 singleton rewrite rules.
Each `PartitionIncrementCheckContext` creates its own instance (line 698),
allocating the rule pipeline each time. For union-all plans with multiple
branches, child contexts each get their own instance. Consider making it a
`static final` constant (e.g., `private static final ExpressionNormalization
NORMALIZATION = new ExpressionNormalization()`) shared across all contexts.
Note: the `ExpressionRewriteContext` reuse (created once in the constructor)
is correct and beneficial — it has only `final` fields.
--
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]
