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

AndNot: avoid returning bitmapcontainer with arraycontainer cardinality #200

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

bpot
Copy link
Collaborator

@bpot bpot commented Aug 1, 2018

In some cases this could cause bitmaps to be serialized incorrectly.

Check if a bitmap needs to be converted to an array container after an
inplace-andNot operation.

This also adds a sanity check when writing bitmap containers to make
sure the cardinality is correct.

seed := int64(42)
rand.Seed(seed)

trials := []trial{
{n: 1010, percentFill: .50, ntrial: 10},
{n: 8192, percentFill: .99, ntrial: 10},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This had to be updated to set more bits or it would trip the new check in writeTo.

@coveralls
Copy link

coveralls commented Aug 1, 2018

Coverage Status

Coverage decreased (-0.01%) to 81.878% when pulling 3c1af42 on bp/iandnot into 4c23670 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 81.878% when pulling 582a13c on bp/iandnot into 4c23670 on master.

In some cases this could cause bitmaps to be serialized incorrectly.

Check if a bitmap needs to be converted to an array container after an
inplace-andNot operation.

This also adds a sanity check when writing bitmap containers to make
sure the cardinality is correct.
@bpot
Copy link
Collaborator Author

bpot commented Aug 1, 2018

We could also validate that array containers have strictly increasing values which, I think, would have caught this on the deserialization side. I'm not sure if we want to be checking that every time we deserialize a bitmap though. I suppose we could add a Verify method of some sort.

@lemire
Copy link
Member

lemire commented Aug 2, 2018

Very good.

@lemire lemire merged commit 3d677d3 into master Aug 2, 2018
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.

3 participants