morrySnow commented on code in PR #42199:
URL: https://github.com/apache/doris/pull/42199#discussion_r1955739668
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -379,4 +397,16 @@ private ImmutableSet<Expression>
getFiltersFromUnionConstExprs(LogicalUnion unio
}
return ImmutableSet.copyOf(filtersFromConstExprs);
}
+
+ private Map<Slot, Expression> generateMap(List<NamedExpression>
namedExpressions) {
Review Comment:
could u add some comment to explain the purpose of this replace map
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -379,4 +397,16 @@ private ImmutableSet<Expression>
getFiltersFromUnionConstExprs(LogicalUnion unio
}
return ImmutableSet.copyOf(filtersFromConstExprs);
}
+
+ private Map<Slot, Expression> generateMap(List<NamedExpression>
namedExpressions) {
+ Map<Slot, Expression> replaceMap = new
LinkedHashMap<>(namedExpressions.size());
+ for (NamedExpression namedExpression : namedExpressions) {
+ if (namedExpression instanceof Alias) {
+ replaceMap.putIfAbsent(namedExpression.toSlot(),
namedExpression.child(0));
+ } else if (namedExpression instanceof SlotReference) {
+ replaceMap.putIfAbsent((Slot) namedExpression,
namedExpression);
Review Comment:
why put a entry with same key and value? i think it is not necessary
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -273,24 +284,31 @@ public ImmutableSet<Expression>
visitLogicalProject(LogicalProject<? extends Pla
public ImmutableSet<Expression> visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Void context) {
return cacheOrElse(aggregate, () -> {
ImmutableSet<Expression> childPredicates =
aggregate.child().accept(this, context);
- // TODO
List<NamedExpression> outputExpressions =
aggregate.getOutputExpressions();
-
- Map<Expression, Slot> expressionSlotMap
- =
Maps.newLinkedHashMapWithExpectedSize(outputExpressions.size());
+ Map<Expression, List<Slot>> expressionSlotMap
+ =
Maps.newHashMapWithExpectedSize(outputExpressions.size());
Review Comment:
not need to keep order anymore?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -273,24 +284,31 @@ public ImmutableSet<Expression>
visitLogicalProject(LogicalProject<? extends Pla
public ImmutableSet<Expression> visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Void context) {
return cacheOrElse(aggregate, () -> {
ImmutableSet<Expression> childPredicates =
aggregate.child().accept(this, context);
- // TODO
List<NamedExpression> outputExpressions =
aggregate.getOutputExpressions();
-
- Map<Expression, Slot> expressionSlotMap
- =
Maps.newLinkedHashMapWithExpectedSize(outputExpressions.size());
+ Map<Expression, List<Slot>> expressionSlotMap
+ =
Maps.newHashMapWithExpectedSize(outputExpressions.size());
for (NamedExpression output : outputExpressions) {
- if (hasAgg(output)) {
- expressionSlotMap.putIfAbsent(
- output instanceof Alias ? output.child(0) :
output, output.toSlot()
- );
+ if (output instanceof Alias &&
supportPullUpAgg(output.child(0))) {
+ expressionSlotMap.computeIfAbsent(output.child(0).child(0),
+ k -> new ArrayList<>()).add(output.toSlot());
+ }
Review Comment:
maybe need add some comment to explain when `supportPullUpAgg` return true,
`output.child(0)` always has one child
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -68,9 +77,11 @@ public class PullUpPredicates extends
PlanVisitor<ImmutableSet<Expression>, Void
Map<Plan, ImmutableSet<Expression>> cache = new IdentityHashMap<>();
private final boolean getAllPredicates;
+ private final ExpressionRewriteContext rewriteContext;
Review Comment:
could we add more detail about this rule in the class' comment or in each
function's comment?
--
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]
