Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a degenerate case in topk #16

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

blinsay
Copy link

@blinsay blinsay commented May 8, 2015

I found a degenerate case in topk, added a test for it, and fixed it.

The bug

Suppose you have the following item frequencies:

map[string]int{
        "a": 123,
        "d": 99,
        "e": 87,
        "b": 45,
        "c": 42
}

If the next 6 items in the stream are [c, c, c, c, c, c], the Stream will have a count of 48 for c and 45 for b, but min will still point to "c" when it should be pointing to "b".

At this point if you add a new item, "f", it will replace "c" instead of replacing "b" even though "c" has a higher count.

The fix

Keep a reference to the actual minimum Element at all times using container/heap.

(This is #3 with cleaner commits, but two years later. Sorry about the delay.)

blinsay added 5 commits May 8, 2015 02:10
- the case occurs when an element that was the minimum grows to become
  not the minimum anymore and then another element is added.
@blinsay blinsay mentioned this pull request May 8, 2015
@bmizerany
Copy link
Owner

@blinsay Has this patch worked for you over the last, almost, 8 years?

@blinsay
Copy link
Author

blinsay commented Mar 7, 2023

I haven't used it in a long time. Feel free to close this out.

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.

2 participants