github-actions[bot] commented on code in PR #68496:
URL: https://github.com/apache/doris/pull/68496#discussion_r4120302535
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/SetPreAggStatus.java:
##########
@@ -705,6 +709,31 @@ public PreAggStatus
visitAggregateFunction(AggregateFunction aggregateFunction,
.off(String.format("%s is not supported.",
aggregateFunction.toSql()));
}
+ @Override
+ public PreAggStatus visitMergeCombinator(MergeCombinator
combinator, AggregateType aggregateType) {
+ return checkAggStateCombinator(combinator, aggregateType);
+ }
+
+ @Override
+ public PreAggStatus visitUnionCombinator(UnionCombinator
combinator, AggregateType aggregateType) {
+ return checkAggStateCombinator(combinator, aggregateType);
+ }
+
+ private PreAggStatus checkAggStateCombinator(AggregateFunction
aggregateFunction,
+ AggregateType aggregateType) {
+ // GENERIC merges stored states with the same aggregate
function. A matching
+ // merge/union can consume the partial states directly;
REPLACE cannot.
+ // The caller requires a bare value slot, and the combinator
builder derives
+ // the nested argument types and nullability from that slot's
AggStateType.
+ AggStateType stateType = (AggStateType)
aggregateFunction.child(0).getDataType();
+ String functionName = ((Combinator)
aggregateFunction).getNestedFunction().getName();
+ if (aggregateType == AggregateType.GENERIC &&
stateType.getFunctionName().equals(functionName)) {
Review Comment:
[P2] Keep reservoir states storage-merged before enabling this scan path.
For `Aggregate(percentile_reservoir_merge(s)) -> Scan(AGGREGATE KEY(k), s
AGG_STATE<percentile_reservoir(...)> GENERIC)`, ON bypasses `BlockReader`'s
merge of equal full keys. `ReservoirSampler::merge` is grouping-dependent above
its 8192-sample cap. In a one-phase, multi-rowset query with 40k zeros at k=1
and separate 20k-one and 30k-two states at k=2 (quantile 0.5), the key-ordered
ON merge drops every one sample and returns 0; OFF merges k=2 first and returns
1, the actual median. This creates substantial bias even for an approximate
percentile. Please exclude this nested function until its merge is
grouping-invariant and cover states above the cap in a multi-rowset regression.
--
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]
