xiangfu0 commented on code in PR #19017:
URL: https://github.com/apache/pinot/pull/19017#discussion_r3626348290
##########
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:
Updated in fbed65f51f: `mergeIntoAccumulator` now accepts
`PercentileTDigestAccumulator` directly, and the plain-digest
wrapping/allocation path is removed. Both TDigest producers in this function
establish that invariant.
##########
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:
Updated in fbed65f51f: the override now mirrors the interface with
`@Nullable` on the return value and both intermediate-result parameters.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAccumulator.java:
##########
@@ -476,19 +477,42 @@ public double compression() {
return _compression;
}
+ /// Returns the length of the [#serialize()] bytes rather than the
materialized [MergingDigest] byte size, so
+ /// generic `TDigest` serializers ([#byteSize()] followed by
[#asBytes(ByteBuffer)]) emit the capacity-preserving
+ /// encoding. Re-encoding through a materialized [MergingDigest] can produce
a verbose digest with more centroids
+ /// than a freshly allocated [MergingDigest] of the same compression can
hold, which readers reject with
+ /// [ArrayIndexOutOfBoundsException]. The length is computed without
materializing the bytes by mirroring the
+ /// [#serialize()] branches; the mutations here (flush, capacity
normalization) are idempotent, so the following
+ /// [#asBytes(ByteBuffer)] call writes exactly this many bytes.
@Override
public int byteSize() {
- return toTDigest().byteSize();
+ if (_pendingSerializedTDigest != null) {
+ return _pendingSerializedTDigest.length;
+ }
+ flush();
+ if (requiresCapacityPreservingEncoding()) {
+ return SMALL_HEADER_SIZE + SMALL_CENTROID_SIZE * _numCentroids;
+ }
Review Comment:
Updated in fbed65f51f: `byteSize()` now invokes the same centroid-count
guard as the capacity-preserving writer through a shared helper. I also added a
regression test that verifies the size calculation rejects the same unencodable
state before allocation.
##########
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:
The generic TDigest deserializer returns a plain `MergingDigest`; using it
here would discard the accumulator type and lose the capacity-preserving
serialization path on a subsequent merge/serde round-trip. This override routes
only TDigest intermediates back into `PercentileTDigestAccumulator` and leaves
all other object types on the generic path. I added an inline comment in
fbed65f51f to make that rationale explicit.
--
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]
