refactor(message_pack_format): make module concrete with DTOs + codec trait#47
Closed
GordonYuanyc wants to merge 5 commits into
Closed
refactor(message_pack_format): make module concrete with DTOs + codec trait#47GordonYuanyc wants to merge 5 commits into
GordonYuanyc wants to merge 5 commits into
Conversation
Move PR #36–#45 wire-format-aligned variants out of src/sketches/ into a new src/wrapper/ module so the in-process algorithms and the sketchlib-go-aligned producer types live in separate trees. Add a src/message_pack_format.rs placeholder noting that asap_sketchlib (Rust) and sketchlib-go each maintain their own MessagePack representation, byte-compatible at the envelope level. Files moved: - src/wrapper/{ddsketch,countminsketch,countsketch,hll,kll,countminsketch_topk}.rs carry the wire-format types appended after the per-file banner in PR #36+. - src/wrapper/{hydra_kll,set_aggregator,delta_set_aggregator}.rs are whole-file moves (they were entirely wire-format). CountL2HH stays in src/sketches/countsketch_topk.rs — it's an in-process top-k algorithm used by sketch_framework, not a wire-format type. No semantic changes; cargo test --lib --all-features still 447 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `[`CANONICAL_HASH_SEED_TABLE`]` and `[`CANONICAL_HASH_SEED`]` shortcut links in the module-level `//!` docs failed to resolve under `cargo doc --all-features`, breaking the Rust Docs CI workflow with `-D warnings`. Both items are still defined in this module; the doc text simply uses inline-code formatting now instead of intra-doc links so docgen succeeds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… trait Move all wire-format DTOs (WireFormat, CmsData, CountMinSketchWithHeapSerialized, HydraKllSketchData, KllSketchData, StringSet, DeltaResult) out of individual wrappers and into message_pack_format::dto as the single source of truth. Add a unified Error enum and a MessagePackCodec trait (static-dispatch, zero cost) that every wire-format-aligned wrapper implements. The legacy inherent serialize_msgpack / deserialize_msgpack methods are preserved with their original signatures as thin shims over the trait, so callers and FFI bindings need no changes. KllSketchData and DeltaResult remain pub via re-exports from their wrapper modules. Add tests/msgpack_compat.rs with round-trip and DTO-shape coverage for every type, plus #[ignore]-d slots reserved for cross-language golden-bytes fixtures from sketchlib-go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`[`Error`]`, `[`MessagePackCodec`]`, `[`dto`]` shortcut links inside the module-level `//!` block failed to resolve under `cargo doc --all-features` even though the items are defined in the same module — likely a rustdoc quirk around `pub use` shadowing the shortcut-link namespace. Switch to inline-code formatting so docgen succeeds; the items are still discoverable via the usual rustdoc sidebar. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GordonYuanyc
force-pushed
the
refactor/restructure-modules
branch
from
f82c308 to
38c27b6
Compare
Collaborator
Author
|
Superseded by #46, which took a different direction (split portable/native + dropped the wrapper module and legacy msgpack shims entirely). |
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.
Summary
WireFormat,CmsData,CountMinSketchWithHeapSerialized,HydraKllSketchData,KllSketchData,StringSet,DeltaResult) out of individual wrappers and intomessage_pack_format::dtoas the single source of truth.Errorenum and aMessagePackCodectrait (static-dispatch, zero cost) implemented by every wire-format-aligned wrapper.tests/msgpack_compat.rswith round-trip + DTO-shape coverage and#[ignore]-d slots reserved for cross-language golden-bytes fixtures fromsketchlib-go.Backwards compatibility
The legacy inherent
serialize_msgpack/deserialize_msgpackmethods are preserved with their original signatures as thin shims over the trait — callers and FFI bindings need no changes.KllSketchDataandDeltaResultremainpubvia re-exports from their wrapper modules.Trade-offs considered
CountSketch/DdSketch/HllSketch: declined — their public fields ARE the wire fields, so theMessagePackCodecimpl serializes them directly. Documented inmessage_pack_format::mod.Encode/Decode) — no new dependency onthiserror.Test plan
cargo buildcargo test(394 lib + 12 round-trip + 5 ignored compat tests pass)cargo clippy --all-targetscargo fmt --check🤖 Generated with Claude Code