fix: preserve purged Frequencies state - #191
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes state loss in FrequentItemsSketch when a purge removes all active counters but the sketch still has non-zero stream history/error state. It does so by introducing an internal “virgin sketch” predicate and using that for serialization and merge fast paths, while keeping the public is_empty() semantics unchanged (active-items based).
Changes:
- Add an internal
is_virgin()check and use it to decide when to emit the short “empty” serialization and when to skip merges. - Update serialization to preserve
stream_weightandoffseteven whenactive_items == 0(by emitting the non-empty preamble withactive_items == 0). - Add regression tests covering round-trip serialization stability and merge preservation for the purged-to-zero-active state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| datasketches/src/frequencies/sketch.rs | Introduces is_virgin() and uses it for merge/serialization empty fast paths to preserve purged sketch state. |
| datasketches/tests/serde_tests/frequencies.rs | Strengthens serde regression to assert purged sketches with zero active items retain weight/error state and reserialize stably. |
| datasketches/tests/frequencies_test/update.rs | Adds a merge regression ensuring purged-zero-active sketches still contribute stream/error state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tisonkun
marked this pull request as draft
August 9, 2026 16:44
tisonkun
marked this pull request as ready for review
August 10, 2026 06:41
Member
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.
Summary
Preserve Frequencies stream and error state when a purge removes every active counter.
The public
is_empty()method retains its existing active-item semantics. A new internalis_initial_state()check is used for serialization and merge, so a zero-active sketch with stream history is no longer treated as an unused sketch.The check requires
stream_weight,offset, and the active-item count all to be zero, avoiding state loss when a wrapped weight or an accepted inconsistent image retains other state.Such a sketch uses the existing non-empty preamble with
active_items == 0, preservingstream_weightandoffsetwithout introducing a new wire format.Closes #188.
Regression coverage
The tests deterministically produce a purged sketch with:
They verify that serialization round trips and merges preserve all of those observations, that reserialization is stable, and that a newly created or reset sketch still uses the eight-byte empty representation.
Additional regression coverage sets a serialized stream weight to zero while retaining either active items or offset, and verifies that serialization and merge preserve the remaining state.
Relationship to other implementations
Current Java, C++, and Go share the previous behavior: they define empty from the active-item count, use it to select the short empty serialization, and skip the sketch during merge.
This is therefore a shared reference-family state-loss issue, not a Rust-only format mismatch. Existing Java, C++, and Go deserializers accept a non-empty preamble with zero active items and retain its stream weight and offset on read. However, they still consider that result empty and may collapse it again during their own reserialization or merge. This PR fixes Rust round trips and Rust merges while documenting that remaining cross-language limitation.
Validation
cargo x prepare-testdatacargo x checkcargo x testcargo x lint