Fix partitioning logic for uneven division #318
Merged
Conversation
This file contains 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
|
This pull request has been linked to Clubhouse Story #7184: Invalid partition created. |
aaronwolen
approved these changes
May 5, 2021
There was a bug in the logic of partitioning in that we used ceil not floor when computing the number of element per partition. This meant that if we rounded up and used up all the elements before the requested user partitioning count we would run out of samples/regions and end up with empty partitions. The empty partitions were not handled gracefully and caused the library to incorrectly fetch all headers. Depending on the exact scenario it might have also resulted in a segfault when attempting to set empty ranges on the core library due to accessing non-existent vector elements. The fix for this issue is to use floor when computing element per partition and to just assign the left overs to the first N partitions, where one is given to each partition until we run out of "left over" elements.
aaronwolen
force-pushed
the
sethshelnutt/ch7184/invalid-partition-created
branch
from
75f5652
to
9d6e491
Compare
This verifies uneven division does not produce one or more empty partitions that would previously result in full exports
This verifies that samples are not assigned to more than one partition
This adjusts the index min/max offsets as a function of the current partition index
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.
There was a bug in the logic of partitioning in that we used ceil not floor when computing the number of element per partition. This meant that if we rounded up and used up all the elements before the requested user partitioning count we would run out of samples/regions and end up with empty partitions. The empty partitions were not handled gracefully and caused the library to incorrectly fetch all headers. Depending on the exact scenario it might have also resulted in a segfault when attempting to set empty ranges on the core library due to accessing non-existent vector elements.
The fix for this issue is to use floor when computing element per partition and to just assign the left overs to the first N partitions,
where one is given to each partition until we run out of "left over" elements.