xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4224674916
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TDigestUtils.java:
##########
@@ -35,48 +35,42 @@
/// interior centroids until the verbose representation fits the 3.2 capacity.
This keeps intermediate values readable
/// in both directions during a rolling upgrade without silently narrowing
double-precision values to floats.
///
-/// Pinot explicitly uses [ScaleFunction#K_1] for new and deserialized merging
digests. T-digest 3.3 changed its default
-/// to `K_2`, whose smaller middle-quantile centroid count caused a material
P75 accuracy regression in Pinot's
-/// deterministic 100-million-row workload. `K_1` retains the accuracy profile
of t-digest 3.2 while using the 3.3
-/// implementation and its two-level compression.
+/// Pinot owns the in-memory K1 implementation and retains these legacy bytes
for stored columns and mixed-version
+/// intermediate results. Decoding checks lengths and numerical state before
allocating centroid arrays. Historical
+/// poisoned states remain byte-readable with NaN statistics because their
original distribution cannot be recovered.
+/// Opaque poisoned payloads retain their original centroids even if an old
reader's default array cannot hold them;
+/// these historical exceptions remain readable by Pinot, but are not
guaranteed to survive a reverse upgrade.
+/// Fractional boundary masses below one, and single-centroid masses between
one and two, are retained exactly:
+/// t-digest 3.3's recompression assertion cannot be satisfied for these
inputs without changing their mass.
public final class TDigestUtils {
- private static final int VERBOSE_ENCODING = 1;
- private static final int SMALL_ENCODING = 2;
+ public static final int VERBOSE_ENCODING = 1;
+ public static final int SMALL_ENCODING = 2;
private static final int LEGACY_CAPACITY_PADDING = 10;
- private static final int VERBOSE_HEADER_SIZE = 32;
- private static final int VERBOSE_CENTROID_SIZE = 16;
- private static final int SMALL_HEADER_SIZE = 30;
- private static final int SMALL_CENTROID_SIZE = 8;
- private static final int DEFAULT_MERGE_BUFFER_MULTIPLIER = 5;
+ public static final int VERBOSE_HEADER_SIZE = 32;
+ public static final int VERBOSE_CENTROID_SIZE = 16;
+ public static final int SMALL_HEADER_SIZE = 30;
+ public static final int SMALL_CENTROID_SIZE = 8;
+ public static final int DEFAULT_MERGE_BUFFER_MULTIPLIER = 5;
private TDigestUtils() {
}
- /// Creates a merging digest with Pinot's accuracy-preserving scale function.
+ /// Creates a digest using Pinot's accuracy-preserving K1 implementation.
public static TDigest createMergingDigest(double compression) {
- return configureScaleFunction(new MergingDigest(compression));
+ return PercentileTDigestAccumulator.forLegacyAggregation(compression);
}
- /// Creates a merging digest with the temporary-buffer footprint used by
t-digest 3.2.
- ///
- /// Segment aggregation can keep many digests live at once. T-digest 3.3
more than doubled the default temporary
- /// buffer to support two-level compression, adding about 11 KiB to every
compression-100 digest. This factory keeps
- /// the 3.3 main-buffer capacity and Pinot's K1 policy, but restores the
smaller temporary buffer for that
- /// high-cardinality use case.
- public static TDigest createMergingDigestWithLegacyBuffer(double
compression) {
- return createMergingDigestWithLegacyBuffer(compression, 0);
+ /// Rejects non-finite compression while retaining the legacy clamping of
finite settings below ten.
+ public static void validateCompression(double compression) {
Review Comment:
**Behavior regression: non-positive compression is now silently clamped
instead of rejected.** At the base commit, the accumulator constructor threw
`IllegalArgumentException("TDigest compression must be positive")` for
`compression <= 0`, so `PERCENTILETDIGEST(col, 95, 0)` or
`PERCENTILE_SMART_TDIGEST(col, 50, 'compression=-5')` failed fast. Now
`validateCompression` only rejects non-finite values and the constructor clamps
to 10, so a mistyped compression silently returns plausible-looking results.
The doc comment ("retaining the legacy clamping of finite settings below ten")
is inaccurate for the `<= 0` range — legacy clamped only (0, 10). Consider
keeping the positivity check in the query-facing constructors and relaxing only
the deserialization path (where historical headers may need tolerance).
--
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]
