xiangfu0 commented on code in PR #19795:
URL: https://github.com/apache/pinot/pull/19795#discussion_r4246616155
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -172,7 +172,7 @@ private void aggregateIntoValueList(int length,
AggregationResultHolder aggregat
} else {
double[][] doubleValues = blockValSet.getDoubleValuesMV();
forEachNotNull(length, blockValSet, (from, toEx) -> {
- for (int i = 0; i < length; i++) {
+ for (int i = from; i < toEx; i++) {
Review Comment:
**Bundled behavior fix — please call it out in the PR description.**
Changing `for (int i = 0; i < length; i++)` to `for (int i = from; i < toEx;
i++)` is a genuine correctness fix (the old loop re-added the entire MV block
once per non-null range, duplicating values and including null rows when null
handling is enabled), and it now has a dedicated regression test. But it
changes `PERCENTILE_SMART_TDIGEST` results for MV columns with null handling
independently of the t-digest replacement, and nothing in the PR/commit message
mentions it. Without a callout:
- upgrade validation may misattribute the result change to the digest
rewrite, and
- a wholesale revert of this dependency-removal PR would silently lose the
fix.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunctionTest.java:
##########
@@ -1185,21 +1352,49 @@ private static byte[][] createSerializedDigests(int
numDigests, int valuesPerDig
IntPredicate useSmallEncoding) {
byte[][] serializedDigests = new byte[numDigests][];
for (int digestIndex = 0; digestIndex < numDigests; digestIndex++) {
- TDigest digest = TDigestUtils.createMergingDigest(compression);
+ TDigest digest =
PercentileTDigestAccumulator.forLegacyAggregation(compression);
for (int valueIndex = 0; valueIndex < valuesPerDigest; valueIndex++) {
digest.add(serializedValue(digestIndex, valueIndex, numDigests,
valuesPerDigest));
}
if (useSmallEncoding.test(digestIndex)) {
- ByteBuffer buffer = ByteBuffer.allocate(digest.smallByteSize());
- digest.asSmallBytes(buffer);
- serializedDigests[digestIndex] = buffer.array();
+ serializedDigests[digestIndex] = compactFixture(digest);
} else {
serializedDigests[digestIndex] =
ObjectSerDeUtils.TDIGEST_SER_DE.serialize(digest);
}
}
return serializedDigests;
}
+ // Legacy input fixtures keep compact decoding coverage without exposing a
compact-writing digest API.
+ private static byte[] compactFixture(TDigest digest) {
+ byte[] serialized = digest.serialize();
+ ByteBuffer input = ByteBuffer.wrap(serialized);
+ if (input.getInt() == TDigestCodec.SMALL_ENCODING) {
+ return serialized;
+ }
+ double min = input.getDouble();
+ double max = input.getDouble();
+ double compression = input.getDouble();
+ int count = input.getInt();
+ int mainCapacity =
Math.max(TDigestCodec.getDefaultCentroidCapacity(compression), count);
+ long bufferCapacity = 5L * mainCapacity;
+ if ((double) (float) compression != compression || count > Short.MAX_VALUE
|| bufferCapacity > Short.MAX_VALUE) {
+ return serialized;
+ }
+ ByteBuffer output = ByteBuffer.allocate(30 + 8 * count);
+
output.putInt(TDigestCodec.SMALL_ENCODING).putDouble(min).putDouble(max).putFloat((float)
compression);
+ output.putShort((short) mainCapacity).putShort((short)
bufferCapacity).putShort((short) count);
+ for (int i = 0; i < count; i++) {
+ float weight = (float) input.getDouble();
+ float mean = (float) input.getDouble();
+ if (!(weight > 0.0f) || !Float.isFinite(weight) ||
!Float.isFinite(mean)) {
+ return serialized;
+ }
+ output.putFloat(weight).putFloat(mean);
+ }
+ return output.array();
+ }
+
private static byte[] createVerboseUnitCentroidDigest(int numCentroids,
double compression) {
Review Comment:
**Duplicate fixture builders — reuse the shared helpers.**
This private `createVerboseUnitCentroidDigest` is a byte-for-byte copy of
the package-private
`PercentileRawTDigestAggregationFunctionTest.createVerboseUnitCentroidDigest`
(line 145, already documented as shared with the Smart test). Likewise,
`compactFixture` (line 1369 here) is duplicated nearly verbatim in
`LegacyTDigestCompatibilityTest` (line 151), both hand-rolling the SMALL
encoding with the same `5L * mainCapacity` / float-representability logic.
If one copy is later fixed (e.g., a header or capacity tweak), the tests
stop exercising the same wire format with nothing failing to signal the
divergence. Suggest calling the existing package-private helper here and
hoisting `compactFixture` into a shared test utility.
##########
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
Review Comment:
**CI gate coupling — fragile under test filtering/reshuffles (acknowledged,
flagging for visibility).**
This gate hard-fails when `LegacyTDigestCompatibilityTest` did not run
earlier in the same job (fixtures come from its side effect), and it downloads
`com.tdunning:t-digest` 3.2/3.3 from Maven Central at verify time. The workflow
comments show this is intentional and fail-closed, which is reasonable — but
note two hazards:
- any surefire filtering (`-Dtest=...`), flaky-quarantine exclusion, or
future test-set reshuffle that drops the test from set 2 fails the whole
unit-test job with an error unrelated to the code under test;
- the gate depends on Maven Central availability for an artifact Pinot no
longer declares anywhere, so repo caching for this job needs to stay in place.
A fallback that invokes the fixture-exporting test directly (e.g., `mvnw -pl
pinot-segment-local test -Dtest=LegacyTDigestCompatibilityTest`) when the
manifest is missing would decouple the gate from test-set composition.
--
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]
