xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4216260826
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/TDigest.java: ########## @@ -0,0 +1,71 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.customobject; + +import java.nio.ByteBuffer; +import java.util.Collection; +import java.util.List; + +/// Pinot's mutable t-digest contract using the existing K1 aggregation behavior and legacy byte encodings. +/// +/// Implementations are not safe for concurrent mutation; callers must serialize access to each aggregation state. +public abstract class TDigest { + public abstract void add(double value); + + public abstract void add(double value, int weight); + + public abstract void add(TDigest other); + + public abstract void add(List<? extends TDigest> others); + + public abstract void compress(); + + public abstract long size(); + + /// Returns centroid mass without narrowing fractional or large weights to the legacy long size view. + public double getTotalWeight() { + return size(); + } + + public abstract double cdf(double value); + + public abstract double quantile(double quantile); + + public abstract Collection<Centroid> centroids(); + + public abstract double compression(); + + public abstract int byteSize(); + + public abstract int smallByteSize(); + + public abstract void asBytes(ByteBuffer buffer); + + public abstract void asSmallBytes(ByteBuffer buffer); + + public abstract int centroidCount(); + + public abstract double getMin(); + + public abstract double getMax(); + + /// Immutable centroid view. Counts retain the legacy int API; serialization preserves double weights. + public record Centroid(double mean, int count) { Review Comment: **[API hazard] `Centroid`'s int `count` silently loses the fractional and >2^31 mass this PR works hard to preserve.** `centroids()` narrows with `(int) _centroidWeights[i]`: a fractional weight in (0,1) becomes count 0 (and `add(mean, 0)` throws 'TDigest weight must be positive: 0'), and weights above `Integer.MAX_VALUE` saturate. In-repo callers deliberately bypass this via the serialize-then-reparse idiom — duplicated with near-identical comments in `PercentileTDigestAccumulator.add(TDigest)` and `NonFiniteAwareTDigest.add` — but the PR also names this class as the public migration target for plugins, which will reasonably reconstruct digests from `centroids()` and silently lose mass. Since Pinot now owns both types, a double-weight centroid view (or primitive mean/weight iteration API) would make in-memory merges direct and remove both byte round-trip workarounds. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/TDigest.java: ########## @@ -0,0 +1,71 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.customobject; + +import java.nio.ByteBuffer; +import java.util.Collection; +import java.util.List; + +/// Pinot's mutable t-digest contract using the existing K1 aggregation behavior and legacy byte encodings. +/// +/// Implementations are not safe for concurrent mutation; callers must serialize access to each aggregation state. +public abstract class TDigest { Review Comment: **[design] The declared plugin migration target lives in pinot-segment-local, which carries no compatibility contract.** The PR description says Java plugins 'must migrate Pinot-facing signatures to `org.apache.pinot.segment.local.customobject.TDigest`', but segment-local is an implementation module — the existing `customobject` precedent (QuantileDigest, AvgPair) covers internal objects, not an advertised external contract. This abstract class is dependency-light (java.nio/java.util only) and could live in pinot-segment-spi (or pinot-spi), leaving the accumulator and TDigestUtils in segment-local. As is, third-party plugins compile against internals that can be refactored without an SPI review gate and must drag the segment-local jar onto their classpath, forcing a second migration when the type is eventually promoted. -- 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]
