GH-40943: [Java] Implement RangeEqualsVisitor for StringView #41636
Conversation
java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java
Show resolved
Hide resolved
github-actions
bot
added
awaiting committer review
github-actions
bot
added
awaiting changes
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java
Outdated
Show resolved
Hide resolved
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java
Outdated
Show resolved
Hide resolved
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java
Outdated
Show resolved
Hide resolved
| // inclusion of long string at the start | ||
| assertTrue(visitor.rangeEquals(new Range(2, 2, 4))); | ||
| // inclusion of long string at the end | ||
| assertTrue(visitor.rangeEquals(new Range(4, 4, 4))); |
There was a problem hiding this comment.
we should also test nulls, and ranges that should not be equal
There was a problem hiding this comment.
I added one case for that.
vibhatha
force-pushed
the
stringview-range-equals-visitor
branch
from
d8d140f to
ec55b8b
Compare
github-actions
bot
added
awaiting change review
| // unequal range | ||
| assertTrue(visitor.rangeEquals(new Range(8, 0, 3))); |
There was a problem hiding this comment.
....they're still equal. Let's test ranges that are not supposed to be equal.
There was a problem hiding this comment.
ah right! 🙂 I misunderstood.
There was a problem hiding this comment.
added a few not equal cases for each category except for inclusion of long string at the end, since it is not possible to have that case.
There was a problem hiding this comment.
added null cases, there is a bit of code duplication in testing the ranges, I left it that way for the readability.
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting change review
|
I am working on adding the null one... |
| return false; | ||
| } | ||
|
|
||
| if (!isNull) { |
There was a problem hiding this comment.
@lidavidm there was an unnecessary looping happened. So I added this. Sorry I missed this earlier.
There was a problem hiding this comment.
nit: it's clearer to do if (isNull) { continue; }
| return false; | ||
| } | ||
|
|
||
| if (!isNull) { |
There was a problem hiding this comment.
nit: it's clearer to do if (isNull) { continue; }
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 1c15c88. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…pache#41636) ### Rationale for this change Adding `RangeEqualsVisitor` for StringView as discussed in apache#40943. ### What changes are included in this PR? Including `RangeEqualsVisitor` visitor method and test cases to validate it. ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#40943 Authored-by: Vibhatha Abeykoon <vibhatha@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
Adding
RangeEqualsVisitorfor StringView as discussed in #40943.What changes are included in this PR?
Including
RangeEqualsVisitorvisitor method and test cases to validate it.Are these changes tested?
Yes
Are there any user-facing changes?
No
RangeEqualsVisitorfor StringView #40943