Repository navigation
refactor(ddsketch)!: ASAPv1 only — drop proto, portable and native formats, add goldens - #174
Draft
GordonYuanyc wants to merge 2 commits into
Draft
GordonYuanyc wants to merge 2 commits into
GordonYuanyc wants to merge 2 commits into
Conversation
DDSketch serializes through ASAPv1 only. Removes proto/ddsketch (DDSketchState, DDSketchDelta, DDSketchBucketDelta) and reserves SketchEnvelope field 14; removes message_pack_format::portable::ddsketch (DdSketch, DdSketchDelta, DDSKETCH_GROW_CHUNK, MAX_APPLY_DELTA_SPAN_BUCKETS) with its crate-root re-exports, and the native MessagePackCodec shim. Tests that exercised only the removed formats are deleted; the rest run against DDSketch and ASAPv1. Adds ASAPv1 golden fixtures ddsketch_positive_a001 (metadata version 1) and ddsketch_signed_a001 (metadata version 2) via the asapv1_golden submodule bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Add the empty-state DDSketch golden (ddsketch_empty_a001) and its test; build the golden mirror state with named fields. - Restore a core-only small-n probe that separates ceil(q*n) from floor(q*(n-1)). - Replace the one-variant DdRankConvention with dd_rank_index / dd_order_statistic. - Spec: alpha is carried as given, never re-derived from gamma. - Docs state current facts only; fixture counts defer to the golden README's Coverage section. Co-Authored-By: Claude Opus 5.5 <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.
Part of the ASAPv1-only series: one PR per sketch group, each off
main. This one removes DDSketch's protobuf, portable msgpack and native-shim formats; ASAPv1 (0x05 0x00) is the only DDSketch format left, and it gets its first goldens.Draft because of the open question below.
Removed
proto/ddsketch/ddsketch.proto(DDSketchState,DDSketchDelta,DDSketchBucketDelta) and itstools/gen-protoentry;SketchEnvelope.ddsketch(14) is nowreserved 14; reserved "ddsketch";.message_pack_format::portable::ddsketch:DdSketch(incl.from_raw,to_proto/from_proto, the proto and msgpack delta codecs,quantile_interpolated),DdSketchDelta,DDSKETCH_GROW_CHUNK,MAX_APPLY_DELTA_SPAN_BUCKETS; crate-root re-exports.message_pack_format::native::ddsketch.Added
DDSketch's serde form (no hashing, no new public API):ddsketch_positive_a001(metadata v1; counts across every uint width, int8 offset),ddsketch_signed_a001(v2; negative store, zero count, uint16/int16 offsets),ddsketch_empty_a001(v1; sum 0, min +inf, max −inf). Each is checked in both directions; the reviewer hand-decoded them against spec §3.8.alphais the constructor's argument bit for bit, never re-derived fromgamma.DDSketch+ ASAPv1 (mutation-checked); a deterministic small-n probe pins core's ceil-nearest-rank convention.Open question
Production (ASAPCollector Go → ASAPQuery-backend) sends DDSketch as proto
SketchEnvelope{DDSketchState}plus bareDDSketchDelta. After this PR Rust decodes neither, and ASAPv1 has no DDSketch delta. Options: send full ASAPv1 state per window, specify an ASAPv1 delta kind first, or hold this until the Go DDSketch codec and a delta decision land.Follow-up worth considering: the decoder accepts a v2 payload whose negative store is empty and zero count is 0, which re-encodes as v1 (two encodings for one state); the spec could require v2 only when needed, with fail-closed tests.
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,ddsketch_signed,conformance_kit,--lib -- ddsketch,pbt -- ddsketch::, releasee2e -- quantiles:: windows::eh_ddsketch numeric_types:: octo::). Implemented and reviewed by separate agents; review findings are addressed in the second commit.🤖 Generated with Claude Code