Repository navigation
refactor(kll)!: ASAPv1 only — drop proto, portable and native formats - #167
Closed
GordonYuanyc wants to merge 2 commits into
Closed
GordonYuanyc wants to merge 2 commits into
GordonYuanyc wants to merge 2 commits into
Conversation
KLL and KLLDynamic serialize only as ASAPv1 (kind_ids 0x06 0x00 / 0x06 0x01).
- proto: reserve SketchEnvelope field 13 ("kll"); KLLState and CoinState stay
in kll.proto because hydra.proto's HydraCell still embeds them.
- portable: remove KllSketch/KllSketchData from the crate root, the
sketchlib_kll_* helpers, SketchlibKll, the value-offset codec and
KllProtoItems; portable::kll becomes a crate-private module holding only
the per-cell type HydraKllSketch uses.
- native: remove the MessagePackCodec shims for KLL<T> and KLLDynamic<T>.
- KLL::from_portable_state is removed.
- Tests: the portable KLL PBT laws and the e2e merge-and-wire rank test now
run on KLL<f64> through serialize_to_bytes; proto/portable-only tests go.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- asapv1_golden -> 13797db: kll_dynamic_f64_k200 and kll_dynamic_i64_k200 (kind_id 0x06 0x01, seed 42, no compaction; i64 items cross every msgpack integer width in both signs). - tests/asapv1_golden.rs: all four KLL cases read the golden payload by hand and compare levels, items and coin to the known state; compact decodes compare wire_levels/wire_items/wire_coin to the source; each KLL kind_id is rejected by the other variant's decoder. - KLLDynamic has seeded constructors: correct the wire.rs comment and the Q-KLL-SEED wording; fixture counts in the docs say seven kind_ids. - CHANGELOG: HydraKllSketch's cell type is reachable but unnameable; Added entry for the fixtures. - Re-wrap kll/wire.rs and e2e_testing_harness.md; drop kll.proto from the sketchlib.proto header list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closed
Collaborator
Author
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.
Part of the ASAPv1-only series: one PR per sketch group, each off
main. This one removes KLL's protobuf, portable msgpack and native-shim formats; ASAPv1 (0x06 0x00compact,0x06 0x01dynamic) is the only KLL format left.Draft because of the open questions below.
Removed
SketchEnvelope.kll(13) is nowreserved 13; reserved "kll";.KLLState/CoinStatestay becausehydra.proto'sHydraCellembedsKLLState; they go with the Hydra PR or the final cleanup.KllSketch,KllSketchData.message_pack_format::portable::kllis now crate-private and holds only whatportable/hydra_kll.rsstill needs; removed from it:SketchlibKlland thesketchlib_kll_*/new_sketchlib_kll*/bytes_from_sketchlib_kllhelpers,aggregate_kll,merge_refs,from_portable_state, theMessagePackCodecimpls, the value-offset codec (KLL_SCALE_SWEEP,KllProtoItems, encode/decode), and theunsafe impl Send/Sync.message_pack_format::native::{kll, kll_dynamic}.KLL::from_portable_state.Added
kll_dynamic_f64_k200,kll_dynamic_i64_k200; the i64 one spans every msgpack int width includingi64::MIN/MAX), pushed to sketchlib-golden-bytes; submodule bumped.KLL<f64>(mutation-checked).Open questions
KLLStatecan carry samples asoffset+ decimalvalue_scale+ zigzag-varintresiduals, which sketchlib-go emits whenever every retained sample is exactly representable. ASAPv1 f64 KLL always writes 9 bytes per item, so payloads grow for integer-valued or low-precision decimal data. Options: accept it, add a compact item encoding to the ASAPv1 KLL spec, or have producers useitem_type "i64"where the data allows.SketchEnvelope{KLLState}(full state only); that path must switch to ASAPv1 before the backend takes this version. ASAPCollector-public already sends ASAPv1 KLL.Verification (rustc 1.99)
cargo fmt --check; clippy-D warningsfor all four CI feature sets;RUSTDOCFLAGS="-D warnings" cargo doc; gen-proto drift clean; filtered tests (asapv1_golden,--lib -- kll,--test pbt -- kll:: hydra::,msgpack_compat,xtest_consumer, e2e KLL/Hydra-KLL). Implemented and reviewed by separate agents; review findings are addressed in the second commit.🤖 Generated with Claude Code