Update the branch to date - #80
Merged
Merged
Conversation
add Customize Hash trait
…on docs with current code
add wrapper for CMS+HEAP and CS+HEAP
add CI for actions
f64 counter support
Add merge function in ElasticSketch
…ducer in a temp dir
Carry the full 20-seed list in HLL/CMS metadata (not just hash_profile_id), so a consumer can read the exact seeds and algorithm straight from the bytes with no registry. ~130 bytes/sketch; resolving seeds from the profile id alone is left as a v2 space optimization. Doc + fail-closed unknown-key test updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apply `cargo clippy --fix` for the uninlined_format_args lint across all
targets (lib + tests) so the CI clippy step passes with
`--all-targets --all-features -D warnings`. Purely mechanical
`format!("{}", x)` -> `format!("{x}")` rewrites; no logic changes.
Also update examples/serialize_deserialize.rs to use an i64 CMS counter,
since the CMS wire format now constrains counters to CmsWireCounter
(i64/f64); this restores compilation of the --all-targets build.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
HLL and Count-Min `serialize_to_bytes` were generic over the hasher but hardcoded the standard ProjectASAP profile into the metadata, so a sketch built with a custom hasher would serialize metadata that lied about how it was hashed. Introduce a `HashProfile` trait alongside `SketchHasher` and derive the wire metadata from it: - New `HashProfile` trait in `common/hash.rs`, implemented for `DefaultXxHasher` with the exact standard-profile values (now the single source of truth; the envelope `HASH_*` string constants are removed). - HLL: `standard_hll_metadata` is now `hll_metadata::<DefaultXxHasher>`; the generic wire methods build/validate `hll_metadata::<H>` and are bounded on `H: HashProfile`. HIP stays on `DefaultXxHasher`. Portable path unchanged. - Count-Min: `cms_metadata::<H>` reads the profile (incl. `MATRIX_SEED_INDEX`); wire methods and the native `MessagePackCodec` impl gain `H: HashProfile`. Unprofiled hashers become impossible to serialize (compile error → fail closed). The standard-profile output is byte-identical: all existing round-trips, `native_and_portable_hll_bytes_match`, and the Go-parity goldens still pass. Adds custom-hasher (`AltHasher`) round-trip tests for HLL and CMS verifying distinct, self-describing bytes and that a standard-profile decode rejects custom-profile bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…handling The ASAPv1 hash-spec metadata is now derived from the hasher's HashProfile (hll_metadata::<H> / cms_metadata::<H>) rather than hardcoded. Document that the field values are sourced from the hasher (standard profile = DefaultXxHasher), that custom hash profiles are supported and self-describing, and that the format fails closed on both ends (serialize requires H: HashProfile; decode validates against the target type's profile). Add a Custom hash profiles subsection, clarify Section 2 validation, add a Q-PROFILE decision, and note Go-side handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nfigs) Add "Section 4 — Wire coverage" to docs/asapv1_wire_format.md making the coverage decision explicit: which in-memory HLL and Count-Min configs are wire-eligible, and the concrete function/trait to implement (or conversion to do) for the ones that are not. Includes an HLL coverage table (all variants, precisions, and any H: HashProfile), a Count-Min coverage table, an actionable "If you want X, do Y" table, and the rationale for keeping the wire a small fixed set. Renumber the former Section 4 (Wire encoding rules) to Section 5 and update the two cross-references in Decisions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ic_ids Align the Section 1 kind_id registry family bytes to the ids already committed in sketchlib-go's wire/asapmsgpack/magic_ids.go, correcting the earlier speculative KLL/DDSketch/KMV/CountSketch assignments that conflicted with Go. Extend the registry with new family bytes (0x0a+) for the remaining sketches inventoried in docs/apis.md, add a Status column, and add mapping notes for the CMSHeap/CSHeap, Hydra, and SetAggregator/DeltaResult cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI's stable clippy is newer than the local toolchain and flags `redundant reference in println! argument` at countsketch.rs:470 (`row_slice` already returns a slice). Remove the extra & (and inline the format arg). Was the only newer-clippy error under -D warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… notes - Mermaid erDiagram (Structure/entity view) contrasting ASAPv1's self-contained one-to-one shape with OTel-Arrow's normalized one-to-many fan-out - placeholder payload subsections for every registry kind_id (§3.3–§3.22), with per-sketch design notes - registry: KLL-dynamic row; 'kind_id = algorithm level' framing note Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review findings on PR #65: - CMS deserialize panicked on a crafted zero-dimension payload ([rows,0,[]]) via Vector2D::from_fn -> 0.ilog2(); now rejected with an Err before from_fn - portable HLL decode used 1<<precision (shift-overflow panic on crafted precision); now checked_shl -> Err - add negative tests: CMS zero-dim rejection, CMS unknown-key rejection, HLL precision cross-rejection (P12 bytes vs P14 decoder), HIP kind_id rejected by a Classic decoder Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add shared ASAPv1 golden byte-vectors that machine-prove the Rust and Go wire encodings are byte-identical, closing the "no cross-language goldens" gap both code reviews flagged. Each fixture is built from a fixed, KNOWN raw sketch state (register bytes / matrix values set directly, never hashed), so the golden tests the wire encoding in isolation from the hash functions. Fixtures cover the implemented wire configs: HLL Classic/Ertl-MLE/HIP at P12, Count-Min i64/RegularPath and f64/FastPath. The .hex goldens (asapv1_golden/) are authored by the Rust reference encoder and are checked in byte-identically to sketchlib-go/asapv1_golden/. tests/asapv1_golden.rs builds each fixture from known state, serializes, and asserts == golden, plus asserts deserialize(golden) round-trips. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+ wire.rs (serialization)
Count-Min's matrix dimensions are configuration that shapes the payload (like HLL's precision), so per the spec's config→metadata rule they now live in the descriptor metadata instead of the payload. - CmsMetadata gains `rows: u32` / `cols: u32` as structural params. Canonical field order (the wire contract Go must mirror): metadata_version, hash_profile_id, hash_algorithm, seed_derivation, input_encoding, seed_list, matrix_seed_index, rows, cols, counter_type, mode. - cms_metadata::<H>(..) now takes rows/cols and populates them. - CmsPayload drops rows/cols; the payload is now a 1-element positional array `[counts]` (mirroring HLL Classic's `[registers]`). - decode reads rows/cols from the validated metadata; the zero-dimension guard and the counts.len() == rows*cols check are unchanged. - Regenerated the two CMS goldens (cms_i64_regular_2x3.hex, cms_f64_fast_2x3.hex). The three HLL goldens are unchanged. - Updated docs/asapv1_wire_format.md (§2, §3.2, §5, Decisions) and added a "Code organization" note documenting the mod/wire split. NOTE: the Go copies of the two CMS goldens now differ from Rust and will stay mismatched until Go PR #70 makes the matching rows/cols → metadata change. That is expected and a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- §2 Fields: add an overview table (metadata = Hash spec + Structural params, with each group's role and member fields) above the two detail tables - §2 Encoding: replace the loose 'optional / omit the key' wording (and the incorrect 'unknown keys are skippable', which contradicts deny_unknown_fields) with the accurate model: each sketch has its own fixed metadata schema; every field is required within it; missing/extra keys fail closed - mermaid METADATA entity: annotate the hash-spec vs structural-params grouping Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add §5 Implementation detail (move Validation + the exotic-sketch conversion recipe out of §2/§3 into it). Reflow the whole doc to one-sentence-per-line. Tighten Status to bullets, lead with Layering, drop the redundant three-way overview, trim the registry history, and fix a §3.2 inaccuracy (both FastPath/RegularPath serialize directly; a mode is only 'converted' to change it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add "What this is" intro and "Which parts to read" reading guide - Add a "Terms" section (envelope, metadata, payload, kind_id, hash profile, seed_list, wire-eligible) so terms are defined before use - Clarify the Layering "who authors each part" paragraph (envelope and metadata are authored once but shipped with every sketch) - Replace the structure diagram with an erDiagram: SerializedSketch as an ordered envelope/metadata/payload container, payload fanning out per kind - Redraw the layering text box in ASCII; drop redundant caption and the weak "by goal" reading list Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace all non-ASCII characters with ASCII equivalents (em/en dashes, section signs, arrows, checkmarks, sigma/superscript, <=, etc.) - Rephrase "rather than" and "X, not Y" constructions as positive statements - Drop "2-byte" from the kind_id prose and mark [family, variant] as today's layout, since kind_id is variable-length (kind_id_len is a u8) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…table - Cut redundant restatements of seed_list-inlined, fail-closed, and golden-vectors-lock across Section 2/4/5, Cross-language, and Decisions (keep one canonical statement of each) - Collapse the 20 not-yet-designed payload subsections (3.3-3.22) into a single reference table - Remove the deferred "Wire coverage" HTML-commented block Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
feat(wire): ASAPv1 self-describing wire format (envelope + HLL + Count-Min)
…ss-language framing) Rewrite message_pack_format.md to describe the shared envelope.rs framing, per-sketch <sketch>/wire.rs serialization, and HashProfile-derived metadata, and to mark portable/native as deprecated. Point every wire-touching doc at docs/asapv1_wire_format.md as the single source of truth. Note the serialize helpers are bounded on wire-eligible configs (HLL: HllWireVariant + HashProfile; CMS: CmsWireCounter/CmsWireMode + HashProfile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
docs: align existing docs/ pages with the ASAPv1 wire format
CountMin::estimate on the regular path seeded the running minimum with S::Counter::from(i32::MAX). For counters wider than i32 (i64/i128), any key whose probed cells all exceed 2147483647 never triggered the `v < min` update, so estimate silently returned the seed — capping every large count at exactly 2147483647. Seed the minimum from row 0's probed cell instead, matching the fast path's fast_query_min. Add a regression test covering an i64 sketch with counts of ~35 billion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(cms): stop clamping RegularPath estimates at i32::MAX
Fixes a real correctness gap surfaced while implementing
ASAPQuery-backend's SummaryExecutor (data_plane): the only
heap-bearing frequency wire type this crate exposed was
CountMinSketchWithHeap (portable::countminsketch_topk). data_plane's
query-side reducer reused that same type for BOTH CmsWithHeap and
CountSketchWithHeap sids -- meaning every CountSketchWithHeap query was
being read out with CMS's min-over-rows estimator instead of Count
Sketch's own median-of-signed-rows estimator. The sketches::CSHeap
computational type (median estimator, "mirrors CMSHeap but with Count
Sketch") already existed and was already correct; there was just no
portable/wire-format wrapper for it, unlike every other sketch family.
Adds `message_pack_format::portable::countsketch_topk::CountSketchWithHeap`,
structurally mirroring CountMinSketchWithHeap (same wire shape: {sketch:
{sketch, row_num, col_num}, topk_heap: [{key, value}], heap_size}) but
backed by CSHeap instead of CMSHeap. The wire SHAPE is unchanged from
what CountMinSketchWithHeap already produces/consumes for this data --
this is a new Rust-side reader/builder for the same bytes with correct
math, not a new cross-language protocol; sketchlib-go needs no changes.
Also adds `#[derive(Clone)]` to `sketches::CSHeap` (CMSHeap already had
it; CSHeap was missing it, needed for CountSketchWithHeap::merge_refs's
structural clone -- both of CSHeap's own fields, Count and HHHeap, are
already Clone).
## Test plan
- [x] `cargo test message_pack_format::portable::countsketch_topk` -- 8
new tests, all passing: creation, empty query, merge (+ dimension
mismatch), msgpack round-trip, aggregate_topk (+ empty), and
test_median_estimator_differs_from_cms_style_min -- the actual
point of this addition, proving the median estimator behaves
differently from a CMS-style min estimator on a skewed insert
pattern, not just sharing a struct shape.
- [x] `cargo test` (full workspace) -- 466 unit + 5 asapv1_golden + 12
msgpack_compat (5 ignored) + 4 sketches_go_parity_probe + 1
xtest_consumer + 19 doctests, all passing.
- [x] `cargo clippy --all-targets --all-features -- -D warnings` --
clean.
- [x] `cargo fmt --check` -- clean.
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Add self-describing ASAPv1 serialization for the two KLL quantile
sketches, mirroring the HLL/CMS pattern: compact `KLL` (kind_id
`0x06 0x00`) and `KLLDynamic` (`0x06 0x01`) share one payload
`[levels, items, coin]`, with `serialize_to_bytes` /
`deserialize_from_bytes` living in per-sketch `wire` submodules.
KLL never hashes (it orders raw values with `total_cmp`), so its
metadata carries no hash-spec group — only the structural params
`{metadata_version, k, m, item_type}` (decision Q-KLL). `item_type`
(`f64`/`i64`) is a metadata param, not a separate kind_id (mirrors
CMS `counter_type`). Retained samples use the top-most-level-first
layout that matches sketchlib-go's `KLLState`, so the compact KLL's
leftward-grown L0 is reversed to input order on the wire.
Decode fails closed: kind_id + metadata + item_type checks, level-
layout consistency, coin `remaining_bits <= 64`, and a `2 <= m <= k
<= MAX_CACHEABLE_K` bound so crafted `k`/`m` cannot drive
`compute_max_capacity` into a huge allocation. The nested serde
`Serialize`/`Deserialize` on `KLL` (used by `HydraCounter`) is kept.
Add golden byte-vectors `kll_f64_k200` / `kll_i64_k200` (1..=50,
seed 42, no compaction — deterministic) and cross-language tests;
document the payload in docs/asapv1_wire_format.md §3.3.
KLLDynamic golden is deferred (it has no seeded constructor); the
portable `KllSketch` double-wrap and the Go mirror are follow-ups.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Second-review follow-up. Two decode-side robustness gaps: 1. The `k`/`m` bound added to the ASAPv1 decoder was missing on the retained nested serde `Deserialize` for `KLL` (the `HydraCounter` path, reachable via `Hydra::deserialize_from_bytes` and direct `rmp_serde::from_slice::<KLL<_>>`). A crafted `k` near `usize::MAX` overflowed `compute_max_capacity` / drove a huge allocation. Apply the same `2 <= m <= k <= MAX_CACHEABLE_K` bound plus level-layout validation (so the buffer math can't underflow or index OOB). 2. `validate_kll_payload` (both ASAPv1 variants) accepted a structurally-valid but adversarial level distribution — e.g. many items parked at a high compactor level — that decoded fine but then overflowed the weighted `count()`/`rank()`/`cdf()` (`size * 2^h`) at query time. Add a shared `checked_weighted_count` guard so decode rejects any layout whose weighted count would overflow; a live sketch's weighted count equals its ingested item count, so real bytes are never rejected. Adds regression tests for both (nested-serde crafted `k`, ASAPv1 weighted-count overflow). Full suite + hydra green; clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an optional `seed` key to the KLL ASAPv1 metadata so a decoded sketch keeps clear()-reproducibility instead of falling back to wall-clock. It's construction config, so it lives in the metadata (config→metadata rule), and it's the first optional key in v1: present only when the sketch carries a seed (Some), omitted otherwise (skip_serializing_if + default). The payload's `coin` already carries the RNG's current position (enough to resume compaction); `seed` is what a later clear() re-seeds from. Scope: compact KLL populates and restores it. KLLDynamic has no seed concept and always omits the key — the two variants are deliberately not forced to be symmetric here. A consumer that doesn't use the key (including Go) must still preserve it verbatim on re-encode. Regenerate the kll_f64/kll_i64 goldens (metadata gains seed=42) and add tests: seed present-when-seeded / omitted-when-unseeded, and that it survives a round trip so a decoded sketch's clear() stays deterministic. Document as decision Q-KLL-SEED. Relates to determinism gap tracked in #77 (tumbling KLL uses unseeded init) — that fix is separate (framework-side). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…table Closes a coverage gap from the third review: existing positive round-trip tests all used init_kll_with_seed, so the seed-absent metadata path was exercised on encode but never fully decoded and re-serialized. Add an unseeded decode + byte-stable re-serialize assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The KLL payload section carried a note contrasting the new format with the proto `KLLState` (`proto/kll/kll.proto`). Two reasons to remove it: - `KLLState` is on the way out — it is exactly the layer this ASAPv1 path replaces (SketchEnvelope field 13, hydra.proto, and portable `from_portable_state`). A doc describing the new format should not document the format it retires. - The note described `KLLState` as the "delta-transmission" path, which is not accurate: KLL has no delta proto at all (the delta codec covers DDSketch/HLL/CMS/CountSketch only). The item-order contract above it is unchanged — it is the normative layout spec (index mapping, level 0 in input order, the compact KLL's leftward-growth reversal, and the intra-level ordering caveat), and its reference to sketchlib-go's `KLLState` names the Go struct the cross-language byte layout is anchored to, not the proto format. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
feat(kll): ASAPv1 wire payload for KLL and KLLDynamic
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
5 participants
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.
No description provided.