xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4217139014
########## 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: Moved the implementation-free TDigest contract into pinot-segment-spi and updated every caller plus the migration note. It uses a Java-11-compatible immutable centroid class; both SPI classes are verified as bytecode major 55. Factories and the K1 implementation remain in segment-local. Addressed in [1d9c042a3d](https://github.com/apache/pinot/commit/1d9c042a3d66a4462d7fe991b41f496f4d3dcb7f). All 473 targeted tests and 322 legacy reader cases pass. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/PercentileTDigestAccumulator.java: ########## @@ -939,10 +957,11 @@ private boolean mergeSorted(double[] incomingMeans, double[] incomingWeights, in return false; } + double newTotalWeight = _totalWeight + incomingWeight; Review Comment: Raw and weighted inputs now use one shared K1 merge kernel with implicit unit weights for raw values. The instance boundary normalizer delegates to the shared primitive implementation, and serialized adoption/initialization logic is shared. The independent golden accuracy bounds remain unchanged and pass. Addressed in [1d9c042a3d](https://github.com/apache/pinot/commit/1d9c042a3d66a4462d7fe991b41f496f4d3dcb7f). All 473 targeted tests and 322 legacy reader cases pass. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/PercentileTDigestValueAggregator.java: ########## @@ -184,34 +197,37 @@ private static final class NonFiniteAwareTDigest extends TDigest { private static final int VERBOSE_CENTROID_SIZE = 16; private final TDigest _finiteDigest; - private long _negativeInfinityWeight; - private long _positiveInfinityWeight; + private double _negativeInfinityWeight; Review Comment: Centralized encoding IDs, header/centroid sizes and wire capacity formulas in TDigestUtils. The wrapper uses shared metadata/header readers, and the benchmark helper uses the same constants. Header parsing no longer maintains three independent copies in the wrapper. Addressed in [1d9c042a3d](https://github.com/apache/pinot/commit/1d9c042a3d66a4462d7fe991b41f496f4d3dcb7f). All 473 targeted tests and 322 legacy 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]
