xiangfu0 commented on code in PR #19795: URL: https://github.com/apache/pinot/pull/19795#discussion_r4239654759
########## compatibility-verifier/tdigest-compatibility/LegacyReader.java: ########## @@ -0,0 +1,141 @@ +/** + * 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.MergingDigest; +import com.tdunning.math.stats.TDigest; +import java.nio.ByteBuffer; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + + +/// Runs only against one legacy jar in a separate JVM; Pinot classes are deliberately absent from its classpath. +/// Fractional endpoint masses below one and singleton masses between one and two are excluded: legacy 3.3 cannot +/// recompress them without changing mass. Those preserved-byte limitations are tested by Pinot's unit tests. +/// Initial value parity covers ordinary continuous/empty/singleton inputs and huge-mean fallback. Weighted endpoint +/// or capacity repair can change interpolation, and 3.2 has older singleton rules; those fixtures retain structural, +/// mass, extrema, and monotonicity checks rather than widening the numerical tolerance. +public final class LegacyReader { + private static final double[] QUANTILES = {0, 0.25, 0.5, 0.75, 0.99, 1}; + + private LegacyReader() { + } + + public static void main(String[] args) + throws Exception { + if (!LegacyReader.class.desiredAssertionStatus()) { + throw new IllegalStateException("The legacy compatibility gate requires -ea"); + } + Path directory = Path.of(args[0]); + List<String> manifest = Files.readAllLines(directory.resolve("manifest.tsv")); + assert manifest.size() == 39 : "Missing compatibility fixtures"; Review Comment: Test-coverage gap (direction asymmetry): no test fixture anywhere in the repo contains bytes actually **written by the tdunning library** for Pinot's decoder to read. This gate verifies the opposite direction only (Pinot-written fixtures -> `MergingDigest.fromBytes`; the read/add/compress/rewrite step re-reads tdunning's own rewrite, never feeding it to Pinot). All "legacy format" inputs fed to Pinot's decoder in `LegacyTDigestCompatibilityTest` are handcrafted ByteBuffers written by Pinot test code simulating the wire format, and since no module depends on `com.tdunning:t-digest` anymore, no unit test can produce genuine library bytes. A future change to `readSerializedHeader`/`inspectSerialized` that mishandles a quirk only the real library produces (3.2 weighted-boundary small encodings, 3.3 enlarged-main-array verbose output, header extrema written by old merges) would pass all unit tests and this gate, surfacing only as wrong percentiles or deserialization failures on historical segments after upgrade. Consider committing a small set of genuine 3.2/3.3-written `.bin` fixtures (or having this harness also run Pinot's decoder over the bytes the legacy jars rewrite) so the tdunning->Pinot direction is pinned too. ########## .github/workflows/pinot_unit_tests.yml: ########## @@ -163,6 +163,10 @@ jobs: timeout-minutes: 120 run: | .github/workflows/scripts/pr-tests/.pinot_tests_unit.sh + - name: Verify legacy t-digest readers + if: ${{ matrix.testset == 2 }} Review Comment: Fragile coupling: this step runs only on `matrix.testset == 2`, which must stay hand-synced with the module partition in `.pinot_tests_unit.sh` (its comment notes pinot-segment-local's tests run in set 2 for balance). If the test sets are ever rebalanced so `LegacyTDigestCompatibilityTest` moves to set 1, `run.sh` fails hard on the empty-manifest check under `set -euo pipefail`, turning an unrelated CI-balancing change into a red build. The gate also depends on hand-synced magic counts in three files: `LegacyTDigestCompatibilityTest` asserts the fixture count, `LegacyReader.java` independently asserts `manifest.size() == 39` plus `nativeQuantileCases == 25` — adding one fixture requires coordinated edits in all three. The failures are at least loud rather than silent, but consider driving the counts from the manifest itself (or a shared constant exported into it) and keying the workflow step on fixture presence rather than the testset number — or running the verification inside the Maven lifecycle of pinot-segment-local so it travels with the fixture-producing test. -- 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]
