morrySnow commented on code in PR #63690:
URL: https://github.com/apache/doris/pull/63690#discussion_r3386070536
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggContext.java:
##########
@@ -87,20 +96,22 @@ public PushDownAggContext(List<AggregateFunction>
aggFunctions,
this.passThroughBigJoin = passThroughBigJoin;
this.hasDecomposedAggIf = hasDecomposedAggIf;
this.hasCaseWhen = hasCaseWhen;
+ this.bilateralState = Objects.requireNonNull(bilateralState);
+ for (Map.Entry<AggregateFunction, Alias> entry :
this.aliasMap.entrySet()) {
+ AggregateFunction aggFunction = entry.getKey();
+ ExprId id = entry.getValue().getExprId();
+ Optional<Action> hintAction = EagerAggHints.decide(aggFunction);
+ hintAction.ifPresent(action -> bilateralState.putAction(id,
action));
+ }
+
}
/**
* check validation
* @return true, if groupKeys is not empty and no group by key is in
aggFunctionsInputSlots
*/
public boolean isValid() {
Review Comment:
⚠️ **Javadoc does not match implementation.** The Javadoc of `isValid()`
(line 113) says:
> @return true, if groupKeys is not empty **and no group by key is in
aggFunctionsInputSlots**
But the implementation only checks `!groupKeys.isEmpty()`. Either update the
Javadoc or fix the implementation to also check that no group by key overlaps
with `aggFunctionsInputSlots`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -534,21 +656,382 @@ public Plan visitLogicalRelation(LogicalRelation
relation, PushDownAggContext co
}
private Plan genAggregate(Plan child, PushDownAggContext context) {
- if (context.isValid() && checkStats(child, context)) {
+ if (isPushDisabledByVariable(context)) {
+ context.getBilateralState().registerNoCountSlot(child);
+ return child;
+ }
+ if (checkStats(child, context) || isPushEnabledByVariable(context)) {
List<NamedExpression> aggOutputExpressions = new ArrayList<>();
for (AggregateFunction func : context.getAggFunctions()) {
aggOutputExpressions.add(context.getAliasMap().get(func));
}
+ Alias countStarAlias = null;
+ boolean countStarAlreadyProjected = false;
+ Count countStar = new Count();
Review Comment:
🤔 **`count(*)` is always emitted from `genAggregate()`** (lines 668-680).
Every call unconditionally creates a `Count` alias and adds it to the aggregate
output. For pure one-side pushdown (non-bilateral paths), this `count(*)` is
unused overhead. Consider making it conditional — only emit `count(*)` when the
subtree might participate in bilateral join multiplicity recovery.
--
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]
