xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4223714009
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -756,12 +841,14 @@ private byte[] toVerboseBytes() {
private byte[] toCapacityPreservingBytes() {
checkCapacityPreservingCentroidCount();
- int mainCapacity = Math.min(Short.MAX_VALUE, Math.max(_numCentroids,
_serializedMainCapacity));
- long defaultBufferCapacity =
Math.multiplyExact(DEFAULT_MERGE_BUFFER_MULTIPLIER, (long) mainCapacity);
+ int mainCapacity = Math.min(Short.MAX_VALUE,
+ Math.max(_centroidCapacity, Math.max(_numCentroids,
_serializedMainCapacity)));
+ long defaultBufferCapacity =
Math.multiplyExact(TDigestUtils.DEFAULT_MERGE_BUFFER_MULTIPLIER, (long)
mainCapacity);
int bufferCapacity = Math.toIntExact(Math.min(Short.MAX_VALUE,
Math.max(Math.max((long) _serializedBufferCapacity, mainCapacity +
1L), defaultBufferCapacity)));
- ByteBuffer buffer = ByteBuffer.allocate(SMALL_HEADER_SIZE +
SMALL_CENTROID_SIZE * _numCentroids);
- buffer.putInt(SMALL_ENCODING);
+ ByteBuffer buffer = ByteBuffer.allocate(
+ TDigestUtils.SMALL_HEADER_SIZE + TDigestUtils.SMALL_CENTROID_SIZE *
_numCentroids);
+ buffer.putInt(TDigestUtils.SMALL_ENCODING);
buffer.putDouble(_min);
buffer.putDouble(_max);
buffer.putFloat((float) _compression);
Review Comment:
**[code-review]** toCapacityPreservingBytes narrows means/weights to float
with no exact-representability fallback, so the accumulator's own asSmallBytes
round trip degrades to NaN for huge values
add(1e39); add(2e39); asSmallBytes(buf); deserialize(buf) -> centroid means
narrow to Infinity under finite double header extrema, inspectSerialized sets
needsLegacyFallback, and quantile(0.5) returns NaN (verified by running the
compiled classes: before-serialization q(0.5)=2.0E39, after round trip NaN).
makeLegacyCompatible guards the same narrowing with `(double)(float)mean !=
mean -> reduceVerboseCentroids`, but asSmallBytes/smallByteSize bypass that
guard; a weight > Float.MAX_VALUE additionally throws 'Invalid TDigest centroid
weight' on read.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -1243,19 +1292,12 @@ private void ensureRawBuffer() {
}
private static int getRawBufferSize(long roundedCompression) {
- return (int) Math.min(MAX_RAW_BUFFER_SIZE, Math.max(MIN_RAW_BUFFER_SIZE,
2L * roundedCompression));
- }
-
- private static int getCentroidCapacity(double compression) {
- double normalizedCompression = Math.max(MIN_COMPRESSION, compression);
- int padding = normalizedCompression < 30.0 ?
LOW_COMPRESSION_CAPACITY_PADDING
- : DEFAULT_CENTROID_CAPACITY_PADDING;
- return Math.toIntExact((long) Math.ceil(2.0 * normalizedCompression +
padding));
+ return (int) Math.min(MAX_RAW_BUFFER_SIZE, Math.max(MIN_RAW_BUFFER_SIZE,
2.0 * roundedCompression));
}
private static int getPendingCentroidCapacity(int centroidCapacity) {
- return Math.min(MAX_RAW_BUFFER_SIZE,
- Math.max(MIN_RAW_BUFFER_SIZE,
Math.multiplyExact(DEFAULT_MERGE_BUFFER_MULTIPLIER, centroidCapacity)));
+ return (int) Math.min(MAX_RAW_BUFFER_SIZE,
+ Math.max(MIN_RAW_BUFFER_SIZE, (long)
TDigestUtils.DEFAULT_MERGE_BUFFER_MULTIPLIER * centroidCapacity));
}
private int getPendingInputLimit() {
Review Comment:
**[code-review]** getPendingInputLimit() collapses to 1 at high compression,
degrading ingestion to one full K1 merge per added value (O(n^2))
_pendingCentroidCapacity is capped at MAX_RAW_BUFFER_SIZE=10000 while
_numCentroids at working compression can exceed it. SQL only validates
compressionFactor >= 0 (AggregationFunctionFactory), so PERCENTILETDIGEST(col,
20000) is legal. Measured on the compiled classes with 1M values: compression
100 -> 59 ms; compression 20000 -> 62,371 ms (limit stuck at 1, ~900x). The
weighted/merge path (working compression 2x) collapses from configured
compression >= ~8000. Results stay correct and loops terminate, but
query/ingestion stalls by 2-3 orders of magnitude.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -912,13 +928,8 @@ private void flushIncoming(double compression) {
double[] incomingMeans = _incomingMeans;
double[] incomingWeights = _incomingWeights;
if (!_incomingCentroidsSorted) {
- ensureIncomingSortCapacity(incomingCount);
- Sort.stableSort(_incomingOrder, _incomingMeans, incomingCount);
- for (int i = 0; i < incomingCount; i++) {
- int sourceIndex = _incomingOrder[i];
- _sortedIncomingMeans[i] = _incomingMeans[sourceIndex];
- _sortedIncomingWeights[i] = _incomingWeights[sourceIndex];
- }
+ ensureSortedIncomingCapacity(incomingCount);
+ stableSortIncoming(incomingCount);
incomingMeans = _sortedIncomingMeans;
incomingWeights = _sortedIncomingWeights;
}
Review Comment:
**[code-review]** flushIncoming clears the incoming buffers before
mergeSorted can throw, silently dropping buffered mass on the exception path
`_numIncomingCentroids = 0; _incomingWeight = 0.0;` run before mergeSorted,
whose checkTotalWeight(newTotalWeight) can still throw: the incremental
pre-checks evaluate ((_totalWeight + _incomingWeight) + ...) + weight while the
merge-time check evaluates a differently-associated sum, and near
Double.MAX_VALUE these round differently (MAX + 5e291 == MAX per add, but MAX +
1e292 == Infinity at flush). A caller that catches and reuses the accumulator
then serves quantiles excluding the dropped mass. flush(double) already orders
this correctly (zeroes _numRawValues after the merge) - move the clears after
mergeSorted to match.
--
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]
