xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4221350962
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -696,34 +680,61 @@ public double getMin() {
@Override
public double getMax() {
if (_pendingSerializedTDigest != null) {
- return
ByteBuffer.wrap(_pendingSerializedTDigest).getDouble(Integer.BYTES +
Double.BYTES);
+ return _pendingSerializedMetadata.max();
}
flush();
return _max;
}
- byte[] serialize() {
+ public byte[] serialize() {
+ if (_legacyDegraded) {
+ if (_pendingSerializedTDigest != null) {
+ return _pendingSerializedTDigest.clone();
+ }
+ if (_totalWeight == _legacySerializedTotalWeight && _min ==
_legacySerializedMin
+ && _max == _legacySerializedMax) {
+ return _legacySerializedBytes.clone();
Review Comment:
Replaced the mass/extrema equality heuristic with an explicit mutation flag.
Any logical add or merge canonicalizes degraded output, including zero-net
poisoned input and raw mass that rounds away; untouched original bytes remain
exact. Both merge orders now emit identical canonical bytes.
Addressed in
[6246a60b4b](https://github.com/apache/pinot/commit/6246a60b4bdbaf59deb89986d2644a41178e1a89).
All 483 targeted tests and both legacy-reader checks pass.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/PercentileTDigestValueAggregator.java:
##########
@@ -527,33 +508,21 @@ private byte[] getSerializedBytes(ByteBuffer
scratchBuffer) {
}
ByteBuffer finite = ByteBuffer.wrap(finiteBytes);
- int encoding = finite.getInt();
- finite.position(finite.position() + 2 * Double.BYTES);
- int finiteCentroidCount;
- boolean finiteVerbose;
- if (encoding == VERBOSE_ENCODING) {
- finite.getDouble();
- finiteCentroidCount = finite.getInt();
- finiteVerbose = true;
- } else if (encoding == SMALL_ENCODING) {
- finite.getFloat();
- finite.getShort();
- finite.getShort();
- finiteCentroidCount = finite.getShort();
- finiteVerbose = false;
- } else {
- throw new IllegalStateException("Invalid format for serialized
histogram");
- }
+ SerializedTDigestMetadata metadata =
TDigestUtils.readSerializedHeader(finite, false);
Review Comment:
Compact finite means are now clamped to their original finite header bounds
before widening and adding Infinity tails. Regressions cover both -Infinity
with max=0.3 and +Infinity with a non-float-exact minimum.
Addressed in
[6246a60b4b](https://github.com/apache/pinot/commit/6246a60b4bdbaf59deb89986d2644a41178e1a89).
All 483 targeted tests and both legacy-reader checks pass.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/PercentileRawTDigestAggregationFunctionTest.java:
##########
@@ -155,17 +153,32 @@ static byte[] createSmallUnitCentroidDigest(int
numCentroids, double compression
return buffer.array();
}
- private static void assertLegacyCompatibleShape(byte[] bytes, double
compression) {
+ static void assertLegacyCompatibleShape(byte[] bytes, double compression) {
Review Comment:
Added a durable real-reader gate to Unit Test Set 2. The test exports 35
fresh current-byte fixtures; an isolated JVM for each pinned 3.2/3.3 jar reads,
adds, compresses and rewrites them with assertions enabled, and any failure
fails the job. The jars are test-tool artifacts only, so no Pinot POM/runtime
dependency was restored. Both versions pass locally; the larger 322-case
compatibility matrix also passes.
Addressed in
[6246a60b4b](https://github.com/apache/pinot/commit/6246a60b4bdbaf59deb89986d2644a41178e1a89).
All 483 targeted tests and both legacy-reader checks pass.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/aggregator/PercentileTDigestValueAggregatorTest.java:
##########
@@ -46,9 +52,10 @@ public void testRawValuesTrackMaxVerboseSize(int
compression, int numValues,
for (int i = 1; i < numValues; i++) {
aggregator.applyRawValue(digest, i);
}
- assertEquals(aggregator.getMaxAggregatedValueByteSize(),
expectedMaxByteSize);
+ int registeredBound = aggregator.getMaxAggregatedValueByteSize();
+ assertTrue(registeredBound <= expectedMaxByteSize);
Review Comment:
The expansion test now starts with 100 compact centroids at compression 200
and adds 310 distinct values. It requires the registered 6592-byte bound before
serialization and emitted bytes larger than the compression-10 limit of 512
bytes, so ignoring the input compression fails. Raw sizing rows also assert
their actual registered bounds.
Addressed in
[6246a60b4b](https://github.com/apache/pinot/commit/6246a60b4bdbaf59deb89986d2644a41178e1a89).
All 483 targeted tests and both legacy-reader checks pass.
--
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]
