Repository navigation
Validate: reject runs past the container end, accept non-minimal run containers - #576
Merged
Merged
Conversation
…containers runContainer16.validate() did not check that a run ends within the container: interval16.last() wraps around, so a run from 65000 to 67000 passed, and Maximum() then returned 1464, which Contains denied. Reject it with the new ErrRunIntervalOutOfRange. validate() also rejected a run container that was not the smallest encoding of its values, although AddOffset produces such containers from valid bitmaps. Drop that rule, as CRoaring does and as RoaringBitmap/RoaringBitmap#884 does for Java. ErrRunIntervalSize is kept, deprecated, for compatibility. The deserialization tests named "Corrupts Run Length" did not run-optimize their bitmaps, so they corrupted array containers, and passed only because the harness checked for any error. They now use run containers and check the run end, and both harnesses check the expected error with ErrorIs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This applies to Go the direction taken for Java in RoaringBitmap/RoaringBitmap#884 (issue RoaringBitmap/RoaringBitmap#879):
Validate()checks that a bitmap is well formed, as CRoaring'srun_container_validatedoes, and no longer requires run containers to be the smallest encoding.Changes
runContainer16.validate()never checked this, andinterval16.last()wraps around. A serialized run patched to cover 65,000..67,000 passedValidate().Maximum()then returned 1464, whichContainsdenied.validate()now checks every run first and returns the newErrRunIntervalOutOfRange.validate()rejected a run container that takes more bytes than an array or bitmap container would. The library produces such containers itself. In a random test with 20,000 inputs per operation,AddOffset64produced 107 bitmaps thatValidate()rejected; for example,AddOffset64of the run 0..9 by 65,534 leaves a run of two values in the first container. The rule added no safety: a well-formed run container is bounded by its block either way.ErrRunIntervalSizeis no longer returned. It stays exported and is marked deprecated, so code that refers to it still compiles.Validatenow says that it does not check for the most compact encoding, and points toRunOptimize.Tests
TestBitMapValidationFromDeserializationandTestBitMapValidationFromFrozennever calledRunOptimize(), so they serialized array containers. They passed only because the corruption broke an array's sort order and the harness accepted any error. Now:assert.ErrorIs(t, err, tt.err), so each case must fail for the reason it names.TestIntervalValidationFailingis updated. Its two "too many runs" containers are well formed, so they now pass. It gains cases for a run ending exactly at 65,535 (accepted) and one past it (rejected).TestValidateAcceptsRunContainersThatAreNotTheSmallestEncodingchecks theAddOffset64result above, before and after a serialization round trip.The new and rewritten tests fail without the change.
go test ./...passes, includingroaring64, which uses the same run container.