phase3(parity): un-ignore hll byte-parity test - #252
Merged
Merged
Conversation
Pairs with asap_sketchlib PR #43, which adds a `wire_proto_variant()` accessor on `HllSketch` that returns the proto enum value Go's `HyperLogLog.SerializePortable` writes for a given `HllVariant`. The existing `HLLWrapper::build_state` already mapped `RsHllVariant::Datafusion -> HllVariant::ErtlMle as i32 = 2` correctly, so the only required change here is constructing the wrapper with `HllVariant::Datafusion` instead of `HllVariant::Regular` — that was the byte-divergent factor (Go emits `HLL_VARIANT_DATAFUSION = 2`, Rust was sending `HLL_VARIANT_REGULAR = 1`). The hash path was already aligned: both `sketchlib-go::common.FromBytes` and `asap_sketchlib::HllSketch::update` route through `xxh3_64(seed=seedList[CanonicalHashSeed=5]=0x6a09e667, key)`. This commit: - Removes the `#[ignore]` from `cross_language_parity::hll_byte_parity_with_go`. With asap_sketchlib's fix in place and the wrapper constructed with `HllVariant::Datafusion`, `cargo test --test cross_language_parity hll_byte_parity_with_go` now passes byte-equality against `integration/parity/golden/hll_envelope.bin`. - Updates the surrounding code-comment block to document the variant alignment + hash-seed alignment instead of repeating the (now stale) divergence reason. CountSketch / CMS parity tests remain `#[ignore]`'d — those divergences are tracked in the same upstream issue and 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>
zzylol
added a commit
that referenced
this pull request
May 5, 2026
Two updates rolled into one PR: ## PROGRESS.md — byte-parity completion (closes #243 follow-on) Adds a top-of-file dated section "Cross-language byte-format parity, 5/5 sketches (2026-05-05)" covering the chain of work that closed #243: - asap_sketchlib PRs #43 / #44 / #45 — HLL hash-seed alignment, CountSketch HashSpec / derive_index / derive_sign port (also extracts the shared `common::hashspec` module), CountMinSketch port routing through the same primitives. - ASAPCollector PR #254 — un-ignores `cms_byte_parity_with_go` with the matching FO-mode wrapper rewrite, and fixes a latent inconsistency in the HLL fixture generator that #252 missed (HLL was using `SerializeProtoBytes()` directly while every other sketch goes through `SerializePortable*` + strip + Marshal; the test only "passed" because fixtures are gitignored and rarely regenerated alongside a test run). - Verification: 7/7 cross_language_parity tests pass with `--include-ignored`, including all 5 byte-parity tests + 2 sanity tests, no ignored remaining. Also flags the downstream unblock: `ASAPQuery-backend`'s `edge_runtime_consumes_precompute_rs.rs` HLL / CS / CMS round-trip tests were `#[ignore = "blocked on ASAPCollector#243"]` per `design-phase3-asap-precompute-rs.md` — they should now pass without backend code changes (mechanical un-ignore PR pending, separate work). ## docs/paper-outline.md — five-claim eval framing The "Measurable benefits at every layer" bullet was 4 lines listing benefits without pointing at *how* the paper proves each one. Replaces it with five named evaluation dimensions (transmission bandwidth, edge CPU, edge memory, query accuracy, query latency) plus the combined Pareto headline claim, with explicit evidence-tooling pointers per claim: - bandwidth: `run_e2e_sweep.sh` (P7) bytes columns + `e2e_plots.py` (P9) bandwidth-vs-N + `cardinality_crossover/` single-host pre-compare. - edge CPU: P7 producer-side cpu column + `bench_2node_sim.sh` + the SDK label-axis profile (paper blocker #2). - edge memory: P7 rss column + `bench_soak.sh` (steady-state RSS / heap / fd-count + slope-based leak verdict). - accuracy: `raw_tee.go` (P4) ground truth + `accuracy_reduce.py` (P8) join + `ASAPQuery-backend/TODO.md` "Accuracy-profile library per sketch type" + `sketchlib-bench/docs/DESIGN.md` for bound derivation. - query latency: `promql_replay.py` (P5) p50/p99 + P9 `query_latency_cdf.png` + `plan_transition.py` (P6) transition timings. The combined Pareto claim (`pareto_acc_vs_thru.png` from P9) is named explicitly as the figure the paper's contribution rests on. The "Formal correctness" bullet stays unchanged. Architectural sibling — the existing [`design-asap-edge-framework.md`](design-asap-edge-framework.md) — covers the system shape; this rewrite makes the empirical bar for "done" explicit alongside it. 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
HllSketch::wire_proto_variant()mirroring Go'sportable.go::SerializePortableenum mapping (HyperLogLog -> HLL_VARIANT_DATAFUSION = 2).#[ignore]fromcross_language_parity::hll_byte_parity_with_goand switches the wrapper construction fromHllVariant::RegulartoHllVariant::Datafusionso the proto envelope variant byte (= 2) matches Go's emitted bytes. The hash path was already aligned — bothsketchlib-go::common.FromBytesandasap_sketchlib::HllSketch::updatecallxxh3_64(seed=seedList[CanonicalHashSeed=5]=0x6a09e667, key).#[ignore]'d — tracked in the same upstream issue.Refs #243.
Test plan
cargo test --test cross_language_parity hll_byte_parity_with_go(passes againstintegration/parity/golden/hll_envelope.bin)cargo test --test cross_language_parity(DDSketch + KLL + HLL byte parity all pass; CountSketch + CMS still ignored)cargo testoverasap-precompute-rs(43 + 11 + 5 + 27 = 86 tests pass, 2 ignored)cargo clippy --all-targets -- -D warnings🤖 Generated with Claude Code