-
Notifications
You must be signed in to change notification settings - Fork 106
GH-52: Make RangeEqualsVisitor of RunEndEncodedVector more efficient #761
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
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| int leftLogicalIndex = range.getLeftStart(); | ||
| int rightLogicalIndex = range.getRightStart(); | ||
| while (leftIterator.nextRun() | rightIterator.nextRun()) { |
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.
| while (leftIterator.nextRun() | rightIterator.nextRun()) { | |
| while (leftIterator.nextRun() || rightIterator.nextRun()) { |
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.
I suppose we already checked the vector lengths to be equal, and since we're requiring run lengths to be equal, we don't need to check for any leftover runs after the loop?
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.
@lidavidm The vector length is checked in RangeEqualsVisitor.rangeEqual. If we call vector.visit(RangeEqualsVisitor, Range)` directly, the argument checks are skipped. Using rangeEqual is the best practice, but we seem to be unable to limit users from using another method. Therefore, I'm also considering whether we need to add more boundary checks in Iterator. What do you think?
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.
I think it doesn't hurt to make sure that both iterators are at the end at the end of the loop as a sanity check.
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.
@lidavidm According to your suggestion, I should change it to while (leftIterator.nextRun() && rightIterator.nextRun()) instead of while (leftIterator.nextRun() || rightIterator.nextRun()). The short-circuit feature of '||' will make only leftIterator move forward, while right stays still.
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.
Ah...that would be clearer over using the bitwise operator, yes
| int rightRunLength = Math.min(rightRunEnd, rightRangeEnd) - rightLogicalIndex; | ||
|
|
||
| if (leftRunLength != rightRunLength) { | ||
| if (leftIterator.getRunLength() != rightIterator.getRunLength()) { |
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.
While the original code had this problem too, maybe it's better to check the run length first? That's presumably a cheaper check and so we can bail out earlier.
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.
Modified as you suggested.
|
Ah, the linter is unhappy: |
|
@lidavidm There were 15 checks that failed. Which ones do I need to fix? Which check action does the linter belong to? |
|
All the checks run basically the same build, so you can check any one of them. The linter is part of running maven - did it not error locally? |
|
Same deal. I can push a fix if that's ok? |
|
@lidavidm Thank you, I applied the spotless, but most of the checks failed, seems it's terminated for some reason. |
|
Retrying - I think GHA had an outage |
|
And I think those JNI failures are unrelated...I'll have to find time and take a look. |
What's Changed
Avoid doing a binary search on every step to make the RangeEqualsVisitor of RunEndEncodedVector more efficient.
Closes #52 .