-
Notifications
You must be signed in to change notification settings - Fork 223
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
OR operation results in corrupt bitmaps #358
Comments
@lemire I'll keep digging, but if you have an idea let me know. |
@lemire That is unclear from the documentation, but I dont think thats quite right. If you check the implementations, I also tried updating my reproducer to use |
@lemire Also I should add that I believe this bug is a regression as I only discovered it while testing the upgrade from 0.5.5 to 1.1 in our production shadows. Reverting to 0.5.5 immediately made the issue go away. ... which makes me realize we may be able to narrow down the issue with a git bisect. |
Ok so I debugged it further and I think the problem is ~ something like:
Something in the space between #2 and #3 is wrong though and breaks the Or functionality. My recommendation is to revert that whole P.R, or at least the modification to the |
Ping @jacksonrnewhouse |
@richardartoul Thanks for the analysis. Let us first try to get @jacksonrnewhouse to comment. |
@lemire No problem, thanks for responding so quick! |
Okay, found the bug. The method toArrayContainer() is unsafe if there are more than arrayDefaultMaxSize (4096) elements in it. The code tries very hard to not invoke it in such cases but there is an off-by-one error in runArrayUnionToRuns, introduced at https://github.com/RoaringBitmap/roaring/pull/312/files#diff-e78a8f6657508d16a490f61f55f8b9773edde74a3d36bc0a96686d402f4c5c31R2284. Within an interval the "length" field is really one less than the length, so we can use uint16 as the size. That function adds |
This only triggers when the run container and array container combine to be the full set, as the value wraps around and returns 0, causing it to try and pack the full set into an array container. |
It also needs the returned run container to be [0, 65355] so that it doesn't even populate the backing uint16 slice of the array container. |
Who wants to take a pick at a PR to fix this? Thanks to @richardartoul we have the test. |
I would be happy to do it, but TBH after reading @jnewhouse explanation I think it would be better if someone familiar with the conventions in the codebase did it since it sounds like the root cause was quite subtle. |
Okay, I put together a fix. Sorry for introducing this bug. |
We don’t apologize each time a bug is introduced. |
Ran into this problem. Seems to be rare and possibly related to range containers? But not sure how to debug it.
The text was updated successfully, but these errors were encountered: