Skip to content

refactor(countsketch)!: ASAPv1 only — drop proto, portable and native formats - #165

Closed
GordonYuanyc wants to merge 2 commits into
mainfrom
asapv1-only/cs
Closed

GordonYuanyc wants to merge 2 commits into
mainfrom
asapv1-only/cs

Conversation

@GordonYuanyc

Copy link
Copy Markdown
Collaborator

Part of the ASAPv1-only series: one PR per sketch group, each off main. This one removes Count Sketch's protobuf, portable msgpack and native-shim formats; ASAPv1 (0x04 0x00) is the only Count Sketch format left.

Draft because of the open questions below.

Removed

  • Proto: CountSketchDelta, CountSketchCell; SketchEnvelope.count_sketch (11) is now reserved 11; reserved "count_sketch";. CountSketchState / TopKState / HeapEntry stay because univmon.proto and hydra.proto embed them; they go with those PRs or the final cleanup.
  • message_pack_format::portable::countsketch: CountSketch, CountSketchDelta, COUNT_SKETCH_TOPK_CAPACITY and their crate-root re-exports, including the proto and msgpack delta codecs.
  • message_pack_format::native::countsketch (the MessagePackCodec impl for Count).
  • tests/cs_envelope_golden.hex and the tests that only exercised the removed formats.

Kept / ported

  • The three ASAPv1 goldens (cs_i64_regular_2x4, cs_i64_fast_2x4, cs_i32_regular_2x4) are still checked through native Count.
  • Four property laws that ran on the portable type now run on native Count (merge = concatenation, linearity, negation, ASAPv1 round trip); mutation-checked.
  • New fast_path_string_keys_land_in_go_cells keeps the only check that native fast-path Count puts string keys in the same cells, with the same signs, as Go's Count Sketch (values taken from the deleted proto golden).

Open questions

  1. Deltas. Production (ASAPCollector Go → ASAPQuery-backend) sends Count Sketch as proto SketchEnvelope{CountSketchState} plus bare CountSketchDelta. After this PR Rust decodes neither, and ASAPv1 has no delta kind. Options: send full ASAPv1 state per window, or design an ASAPv1 Count Sketch delta first.
  2. f64 counters. ASAPv1 Count Sketch allows counter_type "i32" / "i64" only; the removed portable type and Go's default use f64. Fractional or 1/p-rescaled weights have no ASAPv1 representation unless the spec gains "f64" (as Count-Min has) or callers move to integer weights.

Downstream

ASAPQuery-backend (via ASAPPlanner's codec crates) decodes the proto full state and deltas; ASAPPlanner/ASAPQuery name the portable type. Nothing outside this repo is changed here.

Verification (rustc 1.99)

cargo fmt --check; clippy -D warnings for all four CI feature sets; RUSTDOCFLAGS="-D warnings" cargo doc; gen-proto drift clean; filtered tests for the touched areas (--lib -- countsketch, --test pbt -- count_sketch::, asapv1_golden, msgpack_compat, sketches_go_parity_probe, xtest_consumer). Implemented and reviewed by separate agents; review findings are addressed in the second commit.

🤖 Generated with Claude Code

GordonYuanyc and others added 2 commits October 2, 2026 00:40
Count Sketch serializes as ASAPv1 only. Removes the portable CountSketch,
CountSketchDelta and COUNT_SKETCH_TOPK_CAPACITY, the native
MessagePackCodec shim for Count, the proto CountSketchDelta and
CountSketchCell messages, and SketchEnvelope's count_sketch field (11,
now reserved). CountSketchState stays: UnivMon and Hydra embed it.

Tests that exercised Count Sketch through the removed formats are dropped
(proto parity probe, msgpack round trip, xtest consumer case, portable e2e
bound) or ported to the native Count (the four portable property laws).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Count<Vector2D<i64>, FastPath> fed the 25 k-a..k-e inserts must hold the
15 signed cells Go's CountSketch wrote for the same stream. Also rewords
the CSHeap portable module doc and the features.md proto list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@GordonYuanyc

Copy link
Copy Markdown
Collaborator Author

Outdated. ASAPv1 encodings and goldens landed in #232 and #235 against the current spec; removing the legacy formats is redone on current main in #206 and #207.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant