-
Notifications
You must be signed in to change notification settings - Fork 242
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
update CramCompressionRecord.isPlaced() to make it APDelta-aware #1284
Merged
Merged
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
77988f0
update isPlaced() to make it APDelta-aware
jmthibault79 de3e149
test updates
jmthibault79 6167a56
oops
jmthibault79 c10e9b3
APDelta -> usePositionDeltaEncoding
jmthibault79 3caa006
clarify that these are unitialized
jmthibault79 c2fbb7c
rename dataprovider
jmthibault79 86eb56e
refactor SliceTests
jmthibault79 cf8faaf
intializeAlignmentBoundaries and comments
jmthibault79 32986fd
oops
jmthibault79 ebf19c7
clarify isPlaced() logic
jmthibault79 fc45b59
this is why we test lol
jmthibault79 42485bc
more cleanup
jmthibault79 7aa2cfc
refactor testBuildStates
jmthibault79 072ae59
Add test_getAlignmentSpan and improve values of test_getAlignmentEnd
jmthibault79 e6cfa0c
combinatoric placedTests
jmthibault79 99cbe36
better unmapped consistency for SliceTests
jmthibault79 7e534d5
assertSpanAndEnd
jmthibault79 bbe4693
alignment span and end tests
jmthibault79 074a288
more comments and test cases
jmthibault79 fc9a585
Note bug 1301 in comments
jmthibault79 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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 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
Oops, something went wrong.
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 problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one seems inverted - it will be false for coordinate sorted. Since no tests failed, we should add one somewhere that verifies this is working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, good catch.
Unfortunately, I believe that it would take me a substantial amount of time to make sense of CramNormalizer and write a reasonable test for it, so I don't think that's feasible with the quick turnaround time we want for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any test in this PR that would have failed before without the fix for APDelta, but passes now so at least we can be sure that we've fixed it ? I assume we didn't have one before this bug was introduced, since no test caught the issue ? Without that, I'm not even sure I'd recognize the failure if I saw it. Ideally we'd create a ticket describing it (that will be fixed by this PR) so we have a record in case anyone sees it using the interim version that had the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The pair of SliceTests cases starting with the comment on line 97 calls this out explicitly.
On a lower level, the combinations in CramCompressionRecordTest.placedTests show that the APDelta flag does affect results, and some would fail without this fix.