Jackie-Jiang commented on code in PR #19017:
URL: https://github.com/apache/pinot/pull/19017#discussion_r3625828736
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -267,28 +267,35 @@ public Object extractGroupByResult(GroupByResultHolder
groupByResultHolder, int
@Override
public Object merge(Object intermediateResult1, Object intermediateResult2) {
if (intermediateResult1 instanceof TDigest) {
- return mergeIntoTDigest((TDigest) intermediateResult1,
intermediateResult2);
+ return mergeIntoAccumulator((TDigest) intermediateResult1,
intermediateResult2);
}
if (intermediateResult2 instanceof TDigest) {
- return mergeIntoTDigest((TDigest) intermediateResult2,
intermediateResult1);
+ return mergeIntoAccumulator((TDigest) intermediateResult2,
intermediateResult1);
}
DoubleArrayList valueList1 = (DoubleArrayList) intermediateResult1;
DoubleArrayList valueList2 = (DoubleArrayList) intermediateResult2;
valueList1.addAll(valueList2);
return valueList1.size() > _threshold ?
convertValueListToTDigest(valueList1) : valueList1;
}
- private static TDigest mergeIntoTDigest(TDigest tDigest, Object
intermediateResult) {
+ private static TDigest mergeIntoAccumulator(TDigest tDigest, Object
intermediateResult) {
Review Comment:
If that is the case, let's change the signature to avoid future
bugs/performance regressions
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -309,6 +324,9 @@ public SerializedIntermediateResult
serializeIntermediateResult(Object o) {
@Override
public Object deserializeIntermediateResult(CustomObject customObject) {
+ if (customObject.getType() ==
ObjectSerDeUtils.ObjectType.TDigest.getValue()) {
Review Comment:
Why adding this?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -267,28 +267,35 @@ public Object extractGroupByResult(GroupByResultHolder
groupByResultHolder, int
@Override
public Object merge(Object intermediateResult1, Object intermediateResult2) {
Review Comment:
Please add nullable annotation to the signature as well
--
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]
