Uh oh!
There was an error while loading. Please reload this page.
GH-52: Make RangeEqualsVisitor of RunEndEncodedVector more efficient - #761
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.
| while (leftIterator.nextRun() | rightIterator.nextRun()) { | |
| while (leftIterator.nextRun() ||rightIterator.nextRun()) { |
There was a problem hiding this comment.
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.
@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.
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.
@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.
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.
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.
Modified as you suggested.
lidavidm
commented
May 21, 2025
Ah, the linter is unhappy: |
ViggoC
commented
May 22, 2025
@lidavidm There were 15 checks that failed. Which ones do I need to fix? Which check action does the linter belong to? |
lidavidm
commented
May 22, 2025
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? |
lidavidm
commented
May 22, 2025
Same deal. I can push a fix if that's ok? |
ViggoC
commented
May 22, 2025
@lidavidm Thank you, I applied the spotless, but most of the checks failed, seems it's terminated for some reason. |
lidavidm
commented
May 22, 2025
Retrying - I think GHA had an outage |
lidavidm
commented
May 22, 2025
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 .