xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4243711183
########## compatibility-verifier/tdigest-compatibility/GenerateRankErrors.java: ########## @@ -0,0 +1,162 @@ +/** + * 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. + */ + +import com.tdunning.math.stats.Centroid; +import com.tdunning.math.stats.TDigest; +import java.util.Arrays; +import java.nio.ByteBuffer; +import java.security.MessageDigest; +import java.util.HexFormat; +import java.util.SplittableRandom; +import org.apache.pinot.segment.local.utils.TDigestUtils; + + +/// Independent legacy rank oracle, run sequentially with the pinned baseline utility and t-digest 3.3 only. +public final class GenerateRankErrors { + private static final double[] QUANTILES = {0, 0.5, 0.75, 0.95, 0.99, 1}; + private static final String[] DISTRIBUTIONS = {"UNIFORM", "SKEWED", "HEAVY_TAIL", "BIMODAL", "DUPLICATE_HEAVY"}; + private static final String[] ORDERS = {"ORIGINAL", "REVERSED", "RANDOMIZED"}; + private static final String[] INPUTS = {"SERVER_LOCAL", "DISTRIBUTED"}; + + private GenerateRankErrors() { + } + + public static void main(String[] args) + throws Exception { + System.out.println("# mergeOrders=" + String.join(",", ORDERS)); + System.out.println("# reducerInputs=" + String.join(",", INPUTS)); + for (int fanIn : new int[]{8, 32, 128}) { + for (int compression : new int[]{20, 100, 1000}) { + for (String distribution : DISTRIBUTIONS) { + for (String order : ORDERS) { + for (String input : INPUTS) { + Reference reference = referenceCase(fanIn, compression, distribution, order, input); + System.out.print(fanIn + "/" + compression + "/" + distribution + "/" + order + "/" + input + + "," + reference.inputSha256()); + for (double error : reference.rankErrors()) { + System.out.print("," + error); + } + System.out.println(); + } + } + } + } + } + } + + private record Reference(String inputSha256, double[] rankErrors) { + } + + private static Reference referenceCase(int fanIn, int compression, String distribution, String order, String input) + throws Exception { + TDigest[] sources = new TDigest[fanIn]; + double[] raw = new double[fanIn * 64]; + for (int i = 0; i < fanIn; i++) { + TDigest digest = TDigestUtils.createMergingDigest(compression); + SplittableRandom random = new SplittableRandom(0x5EEDL + i); + for (int j = 0; j < 64; j++) { + double value = value(distribution, random.nextDouble()); + raw[64 * i + j] = value; + digest.add(value); + } + sources[i] = input.equals("DISTRIBUTED") ? TDigestUtils.deserialize(TDigestUtils.serialize(digest)) : digest; + } + TDigest result = TDigestUtils.createMergingDigest(compression); + int[] sourceIndexes = indexes(fanIn, order); + for (int index : sourceIndexes) { + if (result.size() == 0) { + result = sources[index]; + } else { + result.add(sources[index]); + } + } + // In t-digest 3.3, centroids() calls compress(), forcing the final public-compression pass before quantiles. + for (Centroid centroid : result.centroids()) { + if (!Double.isFinite(centroid.mean()) || centroid.count() <= 0) { + throw new IllegalStateException("Invalid legacy reference centroid"); + } + } + Arrays.sort(raw); + double[] errors = new double[QUANTILES.length]; + for (int i = 0; i < errors.length; i++) { + errors[i] = rankError(raw, result.quantile(QUANTILES[i]), QUANTILES[i]); Review Comment: **[low, plausible] The frozen rank-error oracle may not be reproducible across CPU architectures.** Commit 727a4fd9 made the seeded *inputs* StrictMath-deterministic, but the oracle *values* on this line flow through the legacy 3.3 jar's K_1 merge/quantile math, which uses `Math.sin`/`Math.cos` — JIT-intrinsified and only required to be within 1 ulp, so results may differ across architectures/JDK intrinsics. Regeneration is byte-identical on x86 under JDK 21/25, but if CI runners move to arm64 (or a contributor regenerates on Apple Silicon), `generate-rank-errors.sh`'s diff against the checked-in CSV could fail on every set-2 run with no product defect. Consider either snapping rank errors to a coarser grid than 1e-12, or documenting the reference architecture next to the pinned baseline. ########## 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 \ Review Comment: **[low] The required 5-minute CI gate bootstraps the Maven wrapper from the network on every run.** This `./mvnw` call (and the one in `generate-rank-errors.sh:55`) downloads the wrapper's own Maven distribution into `~/.m2/wrapper`, which is outside the workflow's `~/.m2/repository` cache, and the exported wagon retry settings do not apply to the wrapper bootstrap. A transient CDN hiccup can fail or time out the `timeout-minutes: 5` "Verify legacy t-digest readers" step even though all tests passed. Using the preinstalled `mvn` (as the rest of the workflow does), or extending the cache to `~/.m2/wrapper`, would remove the uncached network dependency from the gate. -- 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]
