xiangfu0 commented on code in PR #20098:
URL: https://github.com/apache/pinot/pull/20098#discussion_r4418423914
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountRawULLAggregationFunction.java:
##########
@@ -41,12 +42,24 @@ public SerializedULL extractFinalResult(@Nullable
UltraLogLog intermediateResult
return new SerializedULL(intermediateResult);
}
+ @Override
public AggregationFunctionType getType() {
- return AggregationFunctionType.DISTINCTCOUNTULL;
+ return AggregationFunctionType.DISTINCTCOUNTRAWULL;
Review Comment:
The fix is correct, but users will see it. In single-stage, an unaliased
`SELECT distinctCountRawULL(col)` now returns the column
`distinctcountrawull(col)` instead of `distinctcountull(col)`, because
`ReducerDataSchemaUtils` builds names from `getResultColumnName()`. The new
factory test asserts this. Could the PR description or release notes call it
out for clients that look up columns by name? It currently only covers
mixed-version safety.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java:
##########
@@ -370,19 +370,21 @@ public ColumnDataType getFinalResultColumnType() {
@Nullable
@Override
public BigDecimal extractFinalResult(@Nullable BigDecimal
intermediateResult) {
- if (intermediateResult == null) {
- return null;
- }
- if (_precision == null) {
- return intermediateResult;
- }
- BigDecimal result = intermediateResult.round(new MathContext(_precision,
RoundingMode.HALF_EVEN));
- return _scale == null ? result : result.setScale(_scale,
RoundingMode.HALF_EVEN);
+ return intermediateResult != null ? round(intermediateResult) : null;
}
@Override
public BigDecimal mergeFinalResult(BigDecimal finalResult1, BigDecimal
finalResult2) {
- return merge(finalResult1, finalResult2);
+ return round(merge(finalResult1, finalResult2));
Review Comment:
Small caveat: rounding the sum of already-rounded partition sums can differ
from rounding the exact total. Example: `SUMPRECISION(col, 1, 0)` with
partition sums 0.5 and 0.5. Each rounds HALF_EVEN to 0, so the merge returns 0,
but the exact total rounds to 1. Final results can't avoid this, but I'd say so
in the javadoc instead of implying it matches `extractFinalResult`.
--
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]
