Skip to content

fix: preserve purged Frequencies state - #191

Merged
tisonkun merged 2 commits into
mainfrom
codex/preserve-frequencies-purge-state
Aug 10, 2026
Merged

fix: preserve purged Frequencies state#191
tisonkun merged 2 commits into
mainfrom
codex/preserve-frequencies-purge-state

Conversation

@tisonkun

@tisonkun tisonkun commented Aug 9, 2026

Copy link
Copy Markdown
Member

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 internal is_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, preserving stream_weight and offset without introducing a new wire format.

Closes #188.

Regression coverage

The tests deterministically produce a purged sketch with:

  • zero active items,
  • total weight 25,
  • maximum error 1, and
  • an upper bound of 1 for an untracked item.

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-testdata
  • cargo x check
  • cargo x test
  • cargo x lint

@tisonkun
tisonkun requested review from PsiACE and a lite review from Copilot August 9, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_weight and offset even when active_items == 0 (by emitting the non-empty preamble with active_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.

Comment thread datasketches/src/frequencies/sketch.rs Outdated
Comment thread datasketches/tests/serde_tests/frequencies.rs
@tisonkun
tisonkun marked this pull request as draft August 9, 2026 16:44
@tisonkun
tisonkun marked this pull request as ready for review August 10, 2026 06:41
@tisonkun

Copy link
Copy Markdown
Member Author

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.

cc @leerho @proost I'd appreicate it if you can check if it's an issue for Java/C++/Go impls also.

@tisonkun
tisonkun merged commit fbe4e93 into main Aug 10, 2026
10 checks passed
@tisonkun
tisonkun deleted the codex/preserve-frequencies-purge-state branch August 10, 2026 23:06
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.

Frequencies serialization and merge lose purged sketch state

2 participants