xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4223011323
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -81,11 +61,15 @@ final class PercentileTDigestAccumulator extends TDigest {
private double[] _incomingWeights;
private double[] _sortedIncomingMeans;
private double[] _sortedIncomingWeights;
- private int[] _incomingOrder;
private SerializedTDigestInput _serializedTDigestInput;
private byte[] _pendingSerializedTDigest;
private double _pendingSerializedTotalWeight = Double.NaN;
+ private SerializedTDigestMetadata _pendingSerializedMetadata;
+ private boolean _legacyDegraded;
Review Comment:
Kept one explicit degraded transition and centralized its warning. Typed
accumulators can receive corrupted serialized inputs after construction, so a
decode-only subclass would still need a live-state transition or a mutable
delegate across those APIs. The smaller change preserves that invariant and
adds zero-weight recovery and mutation regressions, avoiding a broader
representation rewrite in this dependency-removal PR.
Addressed in
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases pass.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunctionTest.java:
##########
@@ -37,17 +39,26 @@
import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
import
org.apache.pinot.core.query.aggregation.function.AggregationFunction.SerializedIntermediateResult;
import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import
org.apache.pinot.segment.local.customobject.PercentileTDigestAccumulator;
import org.apache.pinot.segment.local.utils.TDigestUtils;
import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.customobject.TDigest;
+import org.apache.pinot.segment.spi.customobject.TDigest.Centroid;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.roaringbitmap.RoaringBitmap;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
public class PercentileTDigestAggregationFunctionTest {
private static final ExpressionContext EXPRESSION =
ExpressionContext.forIdentifier("col");
+ private static final Map<String, double[]> LEGACY_REDUCER_RANK_ERRORS =
loadLegacyReducerRankErrors();
Review Comment:
Oracle loading is now memoized lazily, so it no longer runs during
test-class initialization. Loading validates row width, finite error values,
duplicate keys and the recorded merge/input dimensions. Added a checked-in
isolated generator and regeneration recipe using the pinned historical utility
and real 3.3 jar, with no checkout/POM edits; all 45 rows/270 frozen error
values reproduce bit-for-bit.
Addressed in
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases pass.
##########
pinot-perf/src/main/java/org/apache/pinot/perf/aggregation/TDigestBenchmarkUtils.java:
##########
@@ -75,13 +58,18 @@ static byte[] createFixedVerboseBytes(double[]
sortedValues, double compression)
start = 1 + (centroidId - 1) * numInteriorValues /
numInteriorCentroids;
end = 1 + centroidId * numInteriorValues / numInteriorCentroids;
}
- double sum = 0.0;
- for (int valueId = start; valueId < end; valueId++) {
- sum += sortedValues[valueId];
+ double mean = sortedValues[start];
+ for (int valueId = start + 1; valueId < end; valueId++) {
+ double fraction = 1.0 / (valueId - start + 1);
+ double value = sortedValues[valueId];
+ mean = Math.copySign(1.0, mean) == Math.copySign(1.0, value)
Review Comment:
The FIXED_VERBOSE documentation now explicitly notes that the bounded mean
formula changes generated bytes across this migration. Comparisons across that
boundary must reuse the same generated bytes.
Addressed in
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases pass.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java:
##########
@@ -1501,26 +1479,35 @@ private void decode() {
}
int encodedCentroidCount = _numCentroids;
ensureCapacity(Math.addExact(encodedCentroidCount, 2));
- ByteBuffer input = ByteBuffer.wrap(_bytes);
- input.position(_centroidOffset);
- double totalWeight = 0.0;
- if (_centroidSize == VERBOSE_CENTROID_SIZE) {
- for (int i = 0; i < encodedCentroidCount; i++) {
- double weight = input.getDouble();
- _weights[i] = weight;
- _means[i] = input.getDouble();
- totalWeight += weight;
- }
+ if (Double.isNaN(_metadata.totalWeight())) {
+ _metadata = TDigestUtils.inspectSerialized(ByteBuffer.wrap(_bytes),
_metadata, _means, _weights);
+ _totalWeight = _metadata.totalWeight();
} else {
+ // Pending state was already inspected before retaining bytes. Its
later first read only materializes
+ // those validated centroids; regular merged inputs compute flags and
weights in the decode pass above.
+ ByteBuffer input = ByteBuffer.wrap(_bytes);
+ input.position(_centroidOffset);
for (int i = 0; i < encodedCentroidCount; i++) {
- double weight = input.getFloat();
- _weights[i] = weight;
- _means[i] = input.getFloat();
- totalWeight += weight;
+ _weights[i] = _centroidSize == TDigestUtils.VERBOSE_CENTROID_SIZE ?
input.getDouble() : input.getFloat();
+ double mean = _centroidSize == TDigestUtils.VERBOSE_CENTROID_SIZE ?
input.getDouble() : input.getFloat();
+ _means[i] = clamp(mean, _min, _max);
+ }
+ }
+ if (!_metadata.needsLegacyFallback()) {
+ if (_metadata.unorderedMeans()) {
+ it.unimi.dsi.fastutil.Arrays.mergeSort(0, encodedCentroidCount,
Review Comment:
The remaining qualified fastutil call repairs unordered serialized input.
Java cannot import both Arrays classes, and config/checkstyle.xml's
AvoidStaticImport rule does not permit fastutil mergeSort, so the proposed
static import fails the repository's checks. Kept the qualified reference; the
hot incoming sort already uses direct primitive pair merges.
Verified at
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases pass.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunctionTest.java:
##########
@@ -119,7 +121,7 @@ public void
testMaterializedCapacityPreservingStateSerializesLegacyCompatible()
byte[] serialized =
function.serializeIntermediateResult(merged).getBytes();
assertLegacyCompatibleShape(serialized, 20.0);
Review Comment:
Removed the private weaker shape checker. Every Smart TDigest test now uses
the shared Raw TDigest checker, including compact capacities and payload-length
checks.
Addressed in
[33c0b75b60](https://github.com/apache/pinot/commit/33c0b75b60f5ed7561cbdc186e241167992c0a2e).
All 491 selected tests, the 322-case compatibility matrix and 74 real-reader
cases 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]
