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

bugfix:deserialize:cotainers max len should be 2^16+1 #75

Merged
merged 4 commits into from
Nov 9, 2020

Conversation

detailyang
Copy link
Contributor

@detailyang detailyang commented Nov 5, 2020

Hello.

RoaringBitmap's cotainers max len should be u32::max_value() u16::max_value() + 1 rather then u16 in deserialize.

@Kerollmops
Copy link
Member

Hey,

Have you checked that in the RoaringBitmap format specification?
Because IIRC there cannot be more than 2^16 containers where each container contains from 0 to 2^16 numbers.

But maybe you are right :)

@lemire
Copy link
Member

lemire commented Nov 5, 2020

The actual cardinality is between 1 and 2^16, and 2^16 will overflow a 16-bit word. The workaround is to represent the cardinality as a number between 0 and 2^16-1 with the convention that empty containers are prohibited: this works during serialization and deserialization because we never store or load an empty container.

@detailyang
Copy link
Contributor Author

detailyang commented Nov 6, 2020

@lemire @Kerollmops

Sorry. I just recently read the spec https://github.com/RoaringBitmap/RoaringFormatSpec, The container max length should be 2^16 rather than 2 ^ 32 or 2^16-1.

Checkout the test case named test_bitmap_high16bits :)

Many thanks!

@detailyang detailyang changed the title bugfix:deserialize:cotainers max len should be u32 bugfix:deserialize:cotainers max len should be 2^16+1 Nov 6, 2020
@detailyang
Copy link
Contributor Author

@Kerollmops Can you have look at this? I'm sure the u16::max_value is not right :) See the test case named test_bitmap_high16bits

Many thanks

@Kerollmops
Copy link
Member

Yeah, I will take a look at that, thank you for your time :)

bors try

bors bot added a commit that referenced this pull request Nov 9, 2020
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Indeed you have found a bug in the deserialization process, thank you very much! Could you just fix the fmt issue that I suggested to you, please?

To resume your test, you insert big numbers until 2^32, the maximum possible value, these big numbers will force the RoaringBitmap to create a container for each one of those (?), resulting in 2^16 containers. Finally you check that you are able to deserialize them.

To fix this bug, you have modified the condition that was checking for a correct amount of containers to be under 2^16 instead of under 2^16+1.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you!

bors r+

bors bot added a commit that referenced this pull request Nov 9, 2020
75: bugfix:deserialize:cotainers max len should be 2^16+1 r=Kerollmops a=detailyang

Hello.

RoaringBitmap's cotainers max len should be ~u32::max_value()~ u16::max_value() + 1 rather then u16 in deserialize.

Co-authored-by: detailyang <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

Build failed:

  • ci (stable)

Signed-off-by: detailyang <[email protected]>
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

Build succeeded:

@bors bors bot merged commit 32107a4 into RoaringBitmap:master Nov 9, 2020
@bors
Copy link
Contributor

bors bot commented Nov 9, 2020

try

Timed out.

not-jan pushed a commit to not-jan/roaring-rs that referenced this pull request Aug 31, 2022
75: bugfix:deserialize:cotainers max len should be 2^16+1 r=Kerollmops a=detailyang

Hello.

RoaringBitmap's cotainers max len should be ~u32::max_value()~ u16::max_value() + 1 rather then u16 in deserialize.

Co-authored-by: detailyang <[email protected]>
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