xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4243810300
########## 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: Documented the reference CI environment (Ubuntu x86_64, Temurin 25) next to the pinned baseline and in the verifier README, including the distinction between portable StrictMath inputs and legacy Math-derived output. The golden remains unchanged: current Apple Silicon/Temurin 25 regeneration is also byte-identical for all 270 cases, but that is observed evidence rather than a guarantee for every architecture/JVM. Validated at `1a648f4b0cd662a7bd775a9293152a0e9344d5d3`: 552 affected tests, real t-digest 3.2/3.3 bidirectional bytes, the 270-case oracle, applicable precommit checks and independent Pinot review all pass. New-head CI and maintainer approval remain separate gates. ########## 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: Added a dedicated Set 2 Maven-wrapper cache keyed by .mvn/wrapper/maven-wrapper.properties, retaining the repository wrapper convention. Keeping it separate from the shared dependency cache prevents Set 1 from saving an entry before the wrapper is populated. This reuses the distribution on warm runs; a cold cache still needs the bootstrap download. Validated at `1a648f4b0cd662a7bd775a9293152a0e9344d5d3`: 552 affected tests, real t-digest 3.2/3.3 bidirectional bytes, the 270-case oracle, applicable precommit checks and independent Pinot review all pass. New-head CI and maintainer approval remain separate gates. ########## compatibility-verifier/tdigest-compatibility/generate-rank-errors.sh: ########## @@ -0,0 +1,71 @@ +#!/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. +# + +# Regenerate with the pinned legacy utility and 3.3 jar, then compare all case keys, inputs and rank errors. +# Use --update to replace the checked-in reference after intentionally changing the seeded input corpus. +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_BASELINE=f818bae6d75a91228eb3aac95c921bca4aeb2128 +TDIGEST_WORK="$TDIGEST_ROOT/pinot-segment-local/target/tdigest-rank-oracle" +TDIGEST_JAR_DIR="$TDIGEST_ROOT/pinot-segment-local/target/tdigest-legacy-reader" +TDIGEST_JAR="$TDIGEST_JAR_DIR/t-digest-3.3.jar" +TDIGEST_REFERENCE="$TDIGEST_ROOT/pinot-core/src/test/resources/data/tdigest-3.3-k1-rank-errors.csv" +mkdir -p "$TDIGEST_WORK" "$TDIGEST_JAR_DIR" +if ! git -C "$TDIGEST_ROOT" cat-file -e "$TDIGEST_BASELINE^{commit}" 2>/dev/null; then + # Keep shallow CI checkouts bounded without making a developer's full clone shallow. + TDIGEST_FETCH_ARGS=(--no-tags) + if [[ "$(git -C "$TDIGEST_ROOT" rev-parse --is-shallow-repository)" == true ]]; then + TDIGEST_FETCH_ARGS+=(--depth=1) + fi + for TDIGEST_ATTEMPT in 1 2 3; do + if git -C "$TDIGEST_ROOT" -c http.lowSpeedLimit=1 -c http.lowSpeedTime=30 \ + fetch "${TDIGEST_FETCH_ARGS[@]}" https://github.com/apache/pinot.git "$TDIGEST_BASELINE"; then + break + fi + if [[ "$TDIGEST_ATTEMPT" == 3 ]]; then + exit 1 + fi + sleep "$((TDIGEST_ATTEMPT * 5))" + done +fi +git -C "$TDIGEST_ROOT" show \ + "$TDIGEST_BASELINE:pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TDigestUtils.java" \ + > "$TDIGEST_WORK/TDigestUtils.java" +if [[ ! -s "$TDIGEST_JAR" ]]; then + "$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:3.3:jar "-DoutputDirectory=$TDIGEST_JAR_DIR" +fi +javac -cp "$TDIGEST_JAR" -d "$TDIGEST_WORK" \ + "$TDIGEST_WORK/TDigestUtils.java" \ + "$TDIGEST_ROOT/compatibility-verifier/tdigest-compatibility/GenerateRankErrors.java" +# The historical duplicate-boundary assertions abort otherwise valid seeded 3.3 cases. +java -da -cp "$TDIGEST_WORK:$TDIGEST_JAR" GenerateRankErrors > "$TDIGEST_WORK/rank-errors.csv" +if [[ "${1:-}" == --update ]]; then + sed '/^# mergeOrders=/,$d' "$TDIGEST_REFERENCE" > "$TDIGEST_WORK/reference-header.csv" + cat "$TDIGEST_WORK/reference-header.csv" "$TDIGEST_WORK/rank-errors.csv" > "$TDIGEST_REFERENCE" +else + sed -n '/^# mergeOrders=/,$p' "$TDIGEST_REFERENCE" > "$TDIGEST_WORK/checked-in-rank-errors.csv" + diff -u "$TDIGEST_WORK/checked-in-rank-errors.csv" "$TDIGEST_WORK/rank-errors.csv" +fi +printf 'Independent t-digest 3.3 oracle: 270 case/input fingerprints and rank envelopes verified\n' Review Comment: The script now derives the case count from the generated CSV and prints reference updated (unverified) only on --update; verified is emitted only after a successful diff. Checked the real 270-case verify/update/reverify paths and an isolated 90-case corpus, then confirmed a mismatched reference exits nonzero without a verified message. No golden values changed. Validated at `1a648f4b0cd662a7bd775a9293152a0e9344d5d3`: 552 affected tests, real t-digest 3.2/3.3 bidirectional bytes, the 270-case oracle, applicable precommit checks and independent Pinot review all pass. New-head CI and maintainer approval remain separate gates. -- 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]
