yujun777 commented on code in PR #68863:
URL: https://github.com/apache/doris/pull/68863#discussion_r4236782599
##########
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:
Confirmed, and fixed in 83a8641. Neither checker overrode `visitSum0`, so it
fell through `AggregateFunctionVisitor.visitSum0 -> visitAggregateFunction` and
returned `... is not supported.`, turning `PREAGGREGATION` OFF for a
synchronous aggregate materialized view whose `COUNT` is stored as a `SUM`
value column. `sum0` reads a `SUM` value exactly like `sum` does — the two
differ only on an empty input, which pre-aggregation cannot produce — so
`OneValueSlotAggChecker` and `KeyAndValueSlotsAggChecker` now route `Sum` and
`Sum0` through one shared check.
Covered by `SetPreAggStatusTest.testPureValueSlotDispatch` (sum0 over a SUM
value column is ON, over a MAX column is still OFF) plus an `AGGREGATE KEY`
table case added to the MV suite that asserts `PREAGGREGATION: ON` for `SELECT
k, sum0(n) ... GROUP BY k` and golden-files it against the `sum(n)` result.
##########
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:
Confirmed, and fixed in 83a8641. `ExpressionEstimation` had a `visitSum`
override but no `visitSum0`, so a grouped `sum0(mv_count)` fell through
`AggregateFunctionVisitor.visitSum0 -> visitAggregateFunction ->
visitBoundFunction` and came back as `UNKNOWN` with `avgSizeByte = 1` instead
of the 8 bytes of its BIGINT result type — under-sizing a rollup used as the
join build input by 7 bytes per row. `visitSum0` now mirrors `visitSum`, the
same way `visitCount` and `visitAvg` already do.
Covered by `ExpressionEstimationTest.testSumAndSum0Width`, which asserts
both aggregates come back at 8 bytes.
--
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]
