github-actions[bot] commented on code in PR #68863:
URL: https://github.com/apache/doris/pull/68863#discussion_r4236626416
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java:
##########
@@ -188,7 +188,8 @@ public Function constructRollUp(Expression param,
Expression... varParams) {
if (this.isDistinct()) {
return new BitmapUnionCount(param);
} else {
- return new Sum(param);
+ // sum0, not sum: count returns 0 on empty input while sum returns
NULL
+ return new Sum0(param);
Review Comment:
[P2] Preserve storage preaggregation for COUNT rollups over synchronous MVs.
A sync aggregate MV stores COUNT as an AGG_KEYS SUM value. For
`Aggregate(sum0(mv_count)) -> Scan(sync MV)`, the scan starts with
preaggregation unset and SetPreAggStatus checks the rewritten aggregate. Its
OneValueSlotAggChecker accepts Sum on a SUM value but Sum0 falls through to
unsupported, so this change turns preaggregation OFF and forces storage to
merge partial rows before the rollup. Please teach the SUM-compatible
preaggregation checks to accept non-distinct Sum0 and cover the sync MV plan in
a regression; the new async MTMV test does not exercise this path.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java:
##########
@@ -188,7 +188,8 @@ public Function constructRollUp(Expression param,
Expression... varParams) {
if (this.isDistinct()) {
return new BitmapUnionCount(param);
} else {
- return new Sum(param);
+ // sum0, not sum: count returns 0 on empty input while sum returns
NULL
+ return new Sum0(param);
Review Comment:
[P2] Preserve BIGINT width statistics for Sum0 count rollups. The previous
Sum reached ExpressionEstimation.visitSum, which estimates its result at the
BIGINT width of 8 bytes. Sum0 instead falls through visitBoundFunction to
UNKNOWN with avgSizeByte=1. In `Join -> Aggregate(k, sum0(mv_count) AS c) ->
Scan(MV)`, a grouped COUNT rollup used as the join build input is under-sized
by 7 bytes per row. Statistics.computeSize feeds that estimate into the 1 MiB
broadcast-penalty check, so sufficiently large rollups can receive a cheap
broadcast plan. Add a visitSum0 estimate with its result type width, as
visitSum does, and cover the grouped rollup statistic in a planner test.
--
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]
