Jackie-Jiang commented on code in PR #19017:
URL: https://github.com/apache/pinot/pull/19017#discussion_r3616814932
##########
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:
Not introduced in this PR, but could the value be null? The argument is
marked nullable in the interface
##########
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:
Is the first argument always an accumulator?
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunctionTest.java:
##########
@@ -18,8 +18,113 @@
*/
package org.apache.pinot.core.query.aggregation.function;
+import com.tdunning.math.stats.MergingDigest;
+import com.tdunning.math.stats.TDigest;
+import it.unimi.dsi.fastutil.doubles.DoubleArrayList;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.pinot.common.CustomObject;
+import org.apache.pinot.common.request.Literal;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.core.common.ObjectSerDeUtils;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
public class PercentileSmartTDigestAggregationFunctionTest {
+ private static final ExpressionContext EXPRESSION =
ExpressionContext.forIdentifier("col");
+
+ /// The intermediate result must round-trip through the
[PercentileTDigestAccumulator] so that
+ /// capacity-preserving digests (more centroids than a fresh [MergingDigest]
of the same
+ /// compression can hold) are not re-encoded into bytes a receiving server
cannot read.
+ @Test
+ public void
testIntermediateResultRoundTripPreservesCapacityPreservingState() {
+ int numCentroids = 51;
+ double compression = 20.0;
+ byte[] small =
+
PercentileRawTDigestAggregationFunctionTest.createSmallUnitCentroidDigest(numCentroids,
compression, 60, 100);
+ PercentileSmartTDigestAggregationFunction function = new
PercentileSmartTDigestAggregationFunction(
+ List.of(EXPRESSION,
ExpressionContext.forLiteral(Literal.doubleValue(50.0)),
+
ExpressionContext.forLiteral(Literal.stringValue("THRESHOLD=1;COMPRESSION=20"))),
false);
+
+ Object intermediateResult = function.deserializeIntermediateResult(
+ new CustomObject(ObjectSerDeUtils.ObjectType.TDigest.getValue(),
ByteBuffer.wrap(small)));
+ Assert.assertTrue(intermediateResult instanceof
PercentileTDigestAccumulator);
Review Comment:
(minor) Use static import for `Assert` in tests
--
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]
