xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4241453882
########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/tdigest/TDigestCodec.java: ########## @@ -0,0 +1,698 @@ +/** + * 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.tdigest; + +import java.nio.BufferUnderflowException; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; + +/// Compatibility helpers for the serialized t-digest format shared by t-digest 3.2 and 3.3. +/// +/// Version 3.3 requires the first and last centroid to have unit weight whenever a digest is recompressed. Version +/// 3.2 did not maintain that invariant, so an otherwise valid 3.2 digest can fail an assertion after being read by +/// 3.3. Pure reads retain the legacy centroids and interpolation. Mutation and serialization repair their boundaries +/// only when unit endpoints can preserve their total weight and first moment; otherwise unchanged historical bytes +/// remain readable and mutation fails explicitly. +/// Serialization uses the compact form when 3.3's larger low-compression centroid buffer would not fit in a 3.2 +/// default buffer and every centroid is exactly representable in that format. Otherwise, it merges the nearest +/// 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 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. +/// NaN means at an identifiable infinite tail are restored to that infinite bound without changing their weights. +/// Other poisoned states remain byte-readable with NaN statistics because their distribution cannot be recovered. +/// They retain their original bytes; attempting to mutate them or mix them with another distribution fails explicitly. +/// 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. +/// Historical fractional boundary masses below one, and singleton masses between one and two, retain their original +/// bytes. Fresh serialization rejects these boundaries: t-digest 3.3's unit-endpoint assertion cannot be satisfied +/// without changing their mass. Fractional interior weights remain supported. +public final class TDigestCodec { + public static final int VERBOSE_ENCODING = 1; + public static final int SMALL_ENCODING = 2; + private static final int LEGACY_CAPACITY_PADDING = 10; + 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 TDigestCodec() { + } + + /// Reads the compression from a validated header without decoding centroid arrays. + public static double readCompression(byte[] bytes) { + return readSerializedHeader(ByteBuffer.wrap(bytes)).compression(); + } + + /// Rejects non-finite compression while retaining the legacy clamping of finite settings below ten. + public static void validateCompression(double compression) { + if (!Double.isFinite(compression)) { Review Comment: Confirmed against the prior base: those accumulator query paths previously rejected nonpositive compression. This is an intentional behavior change, not preservation of every former query-validation rule. The shared constructors/factories and historical headers consistently retain tdunning 3.3's finite-below-10 clamp. Rejecting <= 0 in this codec would also reject historical headers accepted by the old library. Added an explicit migration note for PERCENTILETDIGEST(..., 0), SmartTDigest compression=-5, the standard SQL factory's unchanged negative-literal rejection, and nonfinite rejection. Existing TDigestCodecTest cases cover all four construction paths and both stored encodings for negative, zero and low-positive compression. See commit 0f393de0a0ee. ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/tdigest/SerializedTDigestInput.java: ########## @@ -0,0 +1,155 @@ +/** + * 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.tdigest; + +import java.nio.ByteBuffer; +import org.apache.pinot.segment.local.customobject.tdigest.TDigestCodec.SerializedTDigestMetadata; + +/// Reusable, invocation-local view of one serialized TDigest for group-by-MV fanout. +/// +/// [#reset(byte\[\])] validates the header once per input row. The first decode combines numerical validation +/// with filling reusable centroid arrays, so all groups for a row merge the same primitive input without sharing +/// mutable accumulator state. A pending accumulator requests an immutable byte snapshot, created at most once per +/// reset +/// and shared by the row's group fanout. The input must remain thread-confined and must not outlive the aggregation +/// call that owns it. Pending-only inputs inspect their bytes without allocating centroid arrays; their first +/// later materialization reads the already validated values without another numerical validation pass. +public final class SerializedTDigestInput { + byte[] _bytes; + byte[] _retainedBytes; + int _numCentroids; + double[] _means; + double[] _weights; + private boolean _decoded; + private boolean _metadataInspected; + + SerializedTDigestMetadata _metadata; + + public void reset(byte[] bytes) { + reset(bytes, TDigestCodec.readSerializedHeader(ByteBuffer.wrap(bytes))); Review Comment: Fixed in 0f393de0a0ee: SerializedTDigestInput reuses mutable parser/header state for byte[] reset, inspect and decode; immutable snapshots are created only for public metadata access or retained pending input. The accumulator reads the mutable state on materialized merge paths. Public immutable metadata and the shared validation rules remain intact. Extended the existing fanout/reset regression to check retained bytes, metadata, weights, extrema and quantiles across subsequent resets. A JDK 25 ThreadMXBean probe using the same 112-byte/5-centroid payload, reusable input, 200k warmup and five 500k-operation samples measured reset 160 -> 0 B/op and decode/inspect/materialized merge 320 -> 0 B/op. This is allocation evidence for that warmed finite payload, not a query latency claim. 538 affected tests plus real 3.2/3.3 bidirectional compatibility and the unchanged 270-case oracle pass. ########## pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java: ########## @@ -354,7 +356,7 @@ public boolean canUseStarTree(Map<String, Object> functionParameters) { protected static TDigest getDefaultTDigest(AggregationResultHolder aggregationResultHolder, int compressionFactor) { TDigest tDigest = aggregationResultHolder.getResult(); if (tDigest == null) { - tDigest = TDigestUtils.createMergingDigest(compressionFactor); + tDigest = PercentileTDigestAccumulator.forLegacyAggregation(compressionFactor); Review Comment: Removed the unused getDefaultTDigest(AggregationResultHolder, int) overload and its Javadoc in 0f393de0a0ee. Repository-wide caller checks found only the four live GroupBy overload call sites, which remain unchanged. The affected aggregation/query suites and core style/license checks pass. ########## compatibility-verifier/tdigest-compatibility/run.sh: ########## @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# +# 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. +# + +# Run after LegacyTDigestCompatibilityTest. Legacy jars are test-tool artifacts, not Pinot dependencies. +set -euo pipefail +# Match the unit-test workflow's Maven Central retry and connection settings, including standalone runs. +export MAVEN_OPTS="${MAVEN_OPTS:-} -Dmaven.wagon.httpconnectionManager.ttlSeconds=25 \ +-Dmaven.wagon.http.retryHandler.count=30 -Dhttp.keepAlive=false -Dmaven.wagon.http.pool=false" +TDIGEST_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +TDIGEST_FIXTURES="$TDIGEST_ROOT/pinot-segment-local/target/tdigest-compat-fixtures" +TDIGEST_WORK="$TDIGEST_ROOT/pinot-segment-local/target/tdigest-legacy-reader" +if [[ ! -s "$TDIGEST_FIXTURES/manifest.tsv" ]]; then + printf 'Missing or empty t-digest fixture manifest: %s. Run LegacyTDigestCompatibilityTest first.\n' \ + "$TDIGEST_FIXTURES/manifest.tsv" >&2 + exit 1 +fi +mkdir -p "$TDIGEST_WORK" +javac -cp "$TDIGEST_ROOT/pinot-segment-local/target/classes" -d "$TDIGEST_WORK" \ + "$TDIGEST_ROOT/compatibility-verifier/tdigest-compatibility/PinotReader.java" +for TDIGEST_VERSION in 3.2 3.3; do + TDIGEST_JAR="$TDIGEST_WORK/t-digest-$TDIGEST_VERSION.jar" + if [[ ! -s "$TDIGEST_JAR" ]]; then + # Resolution uses the workflow-cached Maven repository; retain each immutable copy for subsequent runs. + "$TDIGEST_ROOT/mvnw" -f "$TDIGEST_ROOT/pom.xml" -N -B -ntp \ + org.apache.maven.plugins:maven-dependency-plugin:3.8.1:copy \ + "-Dartifact=com.tdunning:t-digest:$TDIGEST_VERSION:jar" "-DoutputDirectory=$TDIGEST_WORK" + fi + javac -cp "$TDIGEST_JAR" -d "$TDIGEST_WORK" \ + "$TDIGEST_ROOT/compatibility-verifier/tdigest-compatibility/LegacyReader.java" + java -ea -Xmx128m -cp "$TDIGEST_WORK:$TDIGEST_JAR" LegacyReader "$TDIGEST_FIXTURES" "$TDIGEST_VERSION" Review Comment: Changed the legacy JVM flag to -ea:LegacyReader in 0f393de0a0ee; the explicit assertion-enabled self-check remains. Probes against actual 3.2 and 3.3 jars confirm LegacyReader assertions enabled and MergingDigest assertions disabled; omitting -ea still fails the harness guard. The full legacy reader/writer harness and independent 270-case accuracy oracle 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]
