feat(kll): ASAPv1 wire payload for KLL and KLLDynamic#75
Draft
GordonYuanyc wants to merge 4 commits into
Draft
Conversation
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>
GordonYuanyc
force-pushed
the
feat/kll-payload
branch
from
July 20, 2026 04:30
c1602e4 to
464fa49
Compare
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>
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
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.
What
Self-describing ASAPv1 wire serialization for the two KLL quantile sketches, following the HLL/CMS pattern from #65:
KLL→ kind_id0x06 0x00KLLDynamic→ kind_id0x06 0x01Both share one payload
[levels, items, coin].serialize_to_bytes/deserialize_from_bytesnow emit/read the ASAPv1 envelope (replacing the old rmp form), authored in per-sketchwiresubmodules. The nested serdeSerialize/DeserializeonKLL(used byHydraCounter) is preserved unchanged.Design decisions (docs/asapv1_wire_format.md §3.3 + Decisions)
total_cmp), so the hash-spec group (profile id,seed_list, …) has no truthful value. KLL metadata is structural-only:{metadata_version, k, m, item_type, seed?}, and is notHashProfile-derived.item_type(f64/i64) is a metadata param, not a separate kind_id (mirrors CMScounter_type). The two variants share the payload shape and differ only by kind_id.KLLState. Compact KLL grows its buffer leftward with L0 reverse-input, so its encoder reverses L0 back to input order (reusingwire_levels/wire_items);KLLDynamicstores this layout natively. Distinct from the protoKLLStatepath (unchanged).seedis recorded as the one optional metadata key. It's construction config (so metadata, not payload), present only when the sketch carries one, omitted otherwise. The payload'scoinalready carries the RNG's current position (enough to resume compaction);seedis what a laterclear()re-seeds from, so carrying it keepsclear()-reproducibility across a serialize/restore.KLLDynamichas no seed concept and never emits the key — the variants are deliberately not forced symmetric. A consumer that doesn't use it (incl. Go) must still preserve it verbatim on re-encode.Fail-closed decode
kind_id + metadata +
item_typechecks; level-layout consistency (levels[0]==0, monotonic,levels[last]==items.len(),num_levels<=61); coinremaining_bits<=64;2 <= m <= k <= MAX_CACHEABLE_K(so craftedk/mcan't drivecompute_max_capacityinto a huge allocation); and a weighted-count guard (so a structurally-valid but adversarial level distribution can't overflowcount()/rank()/cdf()at query time). The same bound was also applied to the nestedKLLWireserde path (reachable viaHydraCounter).Tests
asapv1_golden/kll_f64_k200/kll_i64_k200(input1..=50, seed 42, below compaction threshold → deterministic; seed now recorded in metadata) + cross-language assertions intests/asapv1_golden.rs.deny_unknown_fields; item-type cross-rejection; inconsistent-levels; out-of-range k/m; weighted-count overflow; nested-serde crafted-k rejection; seed present-when-seeded / omitted-when-unseeded; seed survives round-trip soclear()stays deterministic; unseeded decode + byte-stable re-serialize.clippy --all-targetsclean;fmtclean.Review
Three independent adversarial code-review passes (subagents). Findings fixed:
k/m(ASAPv1 decoder). Fixed + regression test.KLLWireserde path (HydraCounter); MEDIUM: weighted-count overflow on crafted level layouts. Both fixed (harden(kll)) + regression tests.Commits
feat(kll)— ASAPv1 payload for KLL and KLLDynamicharden(kll)— fail closed on crafted dimensions and level layoutsfeat(kll)— record reproducible compaction seed in KLL metadatatest(kll)— assert unseeded compact KLL decodes and round-trips byte-stableDeferred follow-ups
tumbling.rsfrom_configuses unseededKLL::init;KLLConfighas no seed. Framework-side fix, independent of serialization. (KLLDynamicis not wired into tumbling.)init_kll_with_seedonKLLDynamic, an algorithm change) to be deterministic.KLLWire/HydraCounter serde path still dropsseed— intentional; seed survives only the ASAPv1 path. Revisit if Hydra sketches needclear()-reproducibility after a round trip.KllSketchstill double-wraps ({k, sketch_bytes}wheresketch_bytesis ASAPv1). HLL's portable emits bare ASAPv1; KLL not migrated (hydra_kllnestsKllSketchData). Separate cleanup.0x06 0x00/0x06 0x01payload (incl. the optionalseedkey: preserve, don't interpret) and copy thekll_*.hexfixtures.🤖 Generated with Claude Code