phase3(parity): un-ignore ddsketch byte-parity test - #247
Merged
Merged
Conversation
…ugh wire_alpha() Pairs with asap_sketchlib PR #40, which aligns `asap_sketchlib::DdSketch::update`'s bucket-store growth with `sketchlib-go`'s `Buckets.ensure` (chunks of 128, half-chunk-centered seed) and adds a `wire_alpha()` accessor that returns the gamma-roundtripped alpha matching Go's `SerializePortable` output. This commit: - Updates `DDSketchWrapper::build_state` to populate the proto's `alpha` field via `self.sk.wire_alpha()` instead of `self.sk.alpha`, so the emitted `DDSketchState.alpha` bytes match Go's. Without the roundtrip, even with the chunked-128 store layout in place, the proto bytes still diverge on the very first field. - Removes the `#[ignore]` from `cross_language_parity::ddsketch_byte_parity_with_go`. With the upstream fix in place and the wrapper routing through `wire_alpha()`, `cargo test --test cross_language_parity ddsketch_byte_parity_with_go` now passes byte-equality against the `integration/parity/golden/ddsketch_envelope.bin` fixture. KLL / HLL / CountSketch / CMS parity tests remain `#[ignore]`'d — their divergences are tracked in the same upstream issue and will be addressed in subsequent asap_sketchlib PRs. Refs #243. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 5, 2026
…nerator (#254) ## Summary Closes the last byte-parity gap from #243 and fixes a latent inconsistency in the HLL fixture generator that PR #252 (HLL un-ignore) missed. ## CMS un-ignore (the original Phase 3 step 3 follow-up) Pairs with asap_sketchlib PR #45, which ports the wire-format `CountMinSketch::update` / `::estimate` through the shared `asap_sketchlib::common::hashspec` pipeline (`HashSpec::default()` → single XXH3-64-with-seed → per-row `derive_index` over a power-of-two-rounded column mask). With that upstream fix landed the matrix layout now matches Go cell-for-cell. The remaining gap was wrapper-side: the Rust wrapper was emitting a `CountMinState` with `counter_type = FLOAT64` + `counts_float`, while Go's `CountMinSketch.SerializeProtoBytesFO` emits the Frequency-Only payload with `counter_type = INT64` + `counts_int` + per-row `l1` / `l2` norms (the `Sum_*` mode is reserved for weighted streams). - `asap-precompute-rs/src/sketches/cms.rs`: `CMSWrapper::build_state` now emits the FO payload — `counter_type=INT64`, packed sint64 `counts_int`, per-row `l1[r] = sum_c counts[r][c]` and `l2[r] = sum_c counts[r][c]^2` (collapsed forms of Go's `InsertWithHash`-maintained running norms for the unweighted unit-step stream the parity harness drives — the only producer pattern this wire path serves today). `sum_counts` / `sum2_counts` remain empty (Frequency-Only mode). - `asap-precompute-rs/tests/cross_language_parity.rs`: drop the `#[ignore]` reason citing the pre-#45 hash-layer divergence. - `integration/parity/golden_test.go`: switch the CMS fixture generator from `sk.SerializeProtoBytesFO()` to `sk.SerializePortableFO()` + strip `Producer` / `HashSpec` + `proto.Marshal` (matches the DDSketch / KLL / CountSketch pattern established by PRs #247 / #250 / #253). ## HLL fixture generator alignment Discovered while regenerating fixtures end-to-end after the CMS work: the HLL fixture generator was the odd one out — it called `sk.SerializeProtoBytes()` directly (which embeds Producer + HashSpec), while the Rust HLL wrapper sets both fields to `None`. PR #252 un-ignored `hll_byte_parity_with_go` but did not touch `golden_test.go`, so the regenerated fixture diverged from Rust by exactly the 134-byte Producer + HashSpec metadata footprint. The test only "passed" because fixtures are gitignored and developers rarely regenerate-then-test in one step — once you do (`GOLDEN_REGEN=1 go test ... && cargo test --include-ignored ...`), HLL fails. - `integration/parity/golden_test.go`: HLL block now uses `sk.SerializePortable()` + strip `Producer` / `HashSpec` + `proto.Marshal`, matching DDSketch / KLL / CountSketch / CMS. ## Verification - `GOLDEN_REGEN=1 go test -run TestGenerateGoldenFixtures ./integration/parity/...` — passes; HLL fixture goes from 16532 → 16398 bytes (-134 = stripped Producer + HashSpec); CMS fixture is 8275 bytes (Frequency-Only INT64 payload). - `cargo test --release --test cross_language_parity -- --include-ignored` — 7/7 pass: ddsketch, kll, hll, countsketch, cms byte-parity tests + 2 sanity tests, no ignored remaining. - `cargo test --release -p asap-precompute-rs` — full suite green (27 lib tests + 7 parity tests). Closes #243. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pairs with
asap_sketchlibPR #40, which alignsDdSketch::update's bucket-store growth withsketchlib-go's chunk-128 layout and adds awire_alpha()accessor for the gamma-roundtripped alpha.DDSketchWrapper::build_state'salphafield throughself.sk.wire_alpha()so the emittedDDSketchState.alphabytes match Go'sSerializePortableoutput exactly.#[ignore]oncross_language_parity::ddsketch_byte_parity_with_go.KLL / HLL / CountSketch / CMS parity tests stay
#[ignore]'d — their divergences are tracked in ASAPCollector#243 and will be addressed in follow-up asap_sketchlib PRs.Depends on: ProjectASAP/asap_sketchlib#40 (must merge first so the local
path = "../../asap_sketchlib"dep picks upwire_alpha()).Test plan
cargo test --test cross_language_parity ddsketch_byte_parity_with_gopasses byte-equality againstintegration/parity/golden/ddsketch_envelope.bingolden_fixtures_when_present_are_nonempty,rust_wrappers_produce_nonempty_envelopes_for_same_input) passasap-precompute-rsare affected by routing alpha throughwire_alpha()(the value differs by ~5e-17 — well below any quantile-accuracy threshold)asap_sketchlibmain once feat: e2e SDK→Collector performance benchmark of window aggregation #40 lands🤖 Generated with Claude Code