Copilot commented on code in PR #19017:
URL: https://github.com/apache/pinot/pull/19017#discussion_r3611753252
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAccumulator.java:
##########
@@ -476,19 +477,36 @@ 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;
+ }
+ normalizeCentroidCapacity();
+ return VERBOSE_HEADER_SIZE + VERBOSE_CENTROID_SIZE * _numCentroids;
}
@Override
public int smallByteSize() {
return toTDigest().smallByteSize();
}
+ /// Writes the [#serialize()] bytes; see [#byteSize()]. Bytes are always
written in big-endian order (the t-digest
+ /// wire order) regardless of the destination buffer's byte order, unlike
[MergingDigest#asBytes(ByteBuffer)].
@Override
public void asBytes(ByteBuffer buffer) {
- toTDigest().asBytes(buffer);
+ buffer.put(serialize());
Review Comment:
`asBytes(ByteBuffer)` currently calls `serialize()`, which clones the
pending serialized digest and allocates a new byte[] even in the pass-through
case. Because `ObjectSerDeUtils.TDIGEST_SER_DE.serialize()` already allocates
the destination byte[], this introduces an extra allocation + copy per
serialization for accumulators (regression vs prior
`toTDigest().asBytes(buffer)` behavior). At minimum, avoid cloning for the
`_pendingSerializedTDigest` fast-path and write the pending bytes directly into
the destination buffer.
--
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]
