xiangfu0 commented on PR #19795:
URL: https://github.com/apache/pinot/pull/19795#issuecomment-6055065164
Two further findings from this review anchor on lines outside the PR diff,
so they are posted here instead of inline:
**1. [correctness, pre-existing] `PercentileSmartTDigestAggregationFunction`
MV aggregate path ignores the not-null range (line ~176).**
```java
forEachNotNull(length, blockValSet, (from, toEx) -> {
for (int i = 0; i < length; i++) {
valueList.addElements(valueList.size(), doubleValues[i]);
}
});
```
The loop iterates `0..length` instead of `from..toEx`, so with null handling
enabled every MV row (including null rows) is appended once per not-null range:
null rows are included, and with 2+ ranges values are duplicated — wrong
percentile results. The SV branch above and the group-by MV path below both
honor the range correctly. Pre-existing, but it is a concrete wrong-result bug
in a file this PR touches.
**2. [correctness edge]
`PercentileTDigestAggregationFunction.merge()`/`extractFinalResult()` (lines
~275, ~331) truncate fractional-mass digests to empty.**
`merge()` returns the other operand when `intermediateResult1.size() == 0L`
and `extractFinalResult` returns NULL/NaN on `size() == 0L`, but
`PercentileTDigestAccumulator.size()` is `(long) getTotalWeight()`. A legacy
digest with total mass in (0,1) — e.g. the single-centroid weight-0.5 payload
that `serialize()` special-cases to retain exactly per the new "Fractional
boundary masses below one ... are retained exactly" contract — truncates to
`size() == 0` and is silently discarded in merge, and `extractFinalResult`
returns NULL even though `quantile()` is well-defined. The new
`getTotalWeight()` API exists precisely to avoid this narrowing; these checks
should use `getTotalWeight() == 0.0`.
--
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]
