frequent_items_sketch: fix is_empty() after purge clears all counters - #527
Open
FluorineDog wants to merge 1 commit into
Open
FluorineDog wants to merge 1 commit into
FluorineDog wants to merge 1 commit into
Conversation
A purge subtracts the median from all counters and keeps only positive ones, so when all counters equal the median (typical for many low-frequency items), every counter is wiped and num_active drops to 0 while offset and total_weight remain non-zero. is_empty() was defined as num_active == 0, conflating "no retained items" with "no observations", which caused: - serialize() to emit the empty short format, dropping total_weight and offset on the wire; - merge() to early-return, silently discarding the other sketch's total_weight and offset. Reproducer: 193 distinct items at lg_k=8 (capacity 192). The 193rd update triggers a purge with median 1 that clears all counters; the state then reports is_empty()==true, round-trips to total_weight=0/max_error=0 (falsely looking exact), and contributes nothing when merged. Fix: define emptiness as "no observations" (total_weight == 0). The non-empty wire format already supports num_items == 0, so purged states now round-trip with weight and error intact, and merge absorbs them.
This branch has not been deployed
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.
Problem (AI generated)
frequent_items_sketch::is_empty()is defined asmap.get_num_active() == 0, conflating "no retained items" with "no observations".A purge subtracts the median of all counters and keeps only positive ones. When every counter equals the median — typical for streams with many low-frequency items — the purge wipes all counters, leaving
num_active == 0whileoffsetandtotal_weightremain non-zero. Such a state is then misreported as empty, which corrupts two operations:serialize()emits the empty short format, droppingtotal_weightandoffseton the wire. After a round-trip the sketch readstotal_weight == 0andget_maximum_error() == 0— falsely looking like a fresh, exact sketch.merge()early-returns onother.is_empty(), silently discarding the other sketch'stotal_weightandoffset.Minimal reproducer
See unittest in this PR. After serde, data is lost.
Fix
Use the following to implement is_empty instead.