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

Fixing issue 35 #36

Merged
merged 1 commit into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions binaryfusefilter.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package xorfilter

import (
"errors"
"math"
"math/bits"
"sort"
)

type BinaryFuse8 struct {
Expand Down Expand Up @@ -114,13 +114,8 @@ func PopulateBinaryFuse8(keys []uint64) (*BinaryFuse8, error) {
iterations += 1
if iterations > MaxIterations {
// The probability of this happening is lower than the
// the cosmic-ray probability (i.e., a cosmic ray corrupts your system),
// but if it happens, we just fill the fingerprint with ones which
// will flag all possible keys as 'possible', ensuring a correct result.
for i := 0; i < len(filter.Fingerprints); i++ {
filter.Fingerprints[i] = ^uint8(0)
}
return filter, nil
// the cosmic-ray probability (i.e., a cosmic ray corrupts your system).
return nil, errors.New("too many iterations")

Choose a reason for hiding this comment

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

would be better to define this as an exported error and return the error instead.
i.e.

var ErrTooManyIterations = errors.New("too many iterations")
...
return nil, ErrTooManyIterations

when testing this, you can you the explicit exported variable instead of comparing strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Want to issue a pull request?

Choose a reason for hiding this comment

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

my computer isn't set up for that at the moment.. but maybe you could roll this onto the next set of changes you'll make ?

}

blockBits := 1
Expand Down Expand Up @@ -252,7 +247,7 @@ func PopulateBinaryFuse8(keys []uint64) (*BinaryFuse8, error) {
// manage to remove them all. We may simply sort the key to
// solve the issue. This will run in time O(n log n) and it
// mutates the input.
sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] })
keys = pruneDuplicates(keys)
}
for i := uint32(0); i < size; i++ {
reverseOrder[i] = 0
Expand Down
22 changes: 22 additions & 0 deletions binaryfusefilter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"math/rand"
"testing"

"github.com/cespare/xxhash"

Check failure on line 8 in binaryfusefilter_test.go

View workflow job for this annotation

GitHub Actions / single-ver

no required module provides package github.com/cespare/xxhash; to add it:

Check failure on line 8 in binaryfusefilter_test.go

View workflow job for this annotation

GitHub Actions / test (1.16.x, ubuntu-latest)

no required module provides package github.com/cespare/xxhash; to add it:
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -305,3 +306,24 @@
binaryfusedbig.Contains(rand.Uint64())
}
}

func Test_Issue35(t *testing.T) {
for test := 0; test < 100; test++ {
hashes := make([]uint64, 0)
for i := 0; i < 40000; i++ {
v := encode(int32(rand.Intn(10)), int32(rand.Intn(100000)))
hashes = append(hashes, xxhash.Sum64(v))
}
inner, err := PopulateBinaryFuse8(hashes)
if err != nil {
panic(err)
}
for i, d := range hashes {
e := inner.Contains(d)
if !e {
panic(i)
}

}
}
}
207 changes: 0 additions & 207 deletions fusefilter.go

This file was deleted.

Loading
Loading