Skip to content

GH-40943: [Java] Implement RangeEqualsVisitor for StringView - #41636

Merged
lidavidm merged 10 commits into
apache:mainfrom
vibhatha:stringview-range-equals-visitor
May 16, 2024
Merged

GH-40943: [Java] Implement RangeEqualsVisitor for StringView#41636
lidavidm merged 10 commits into
apache:mainfrom
vibhatha:stringview-range-equals-visitor

Conversation

@vibhatha

@vibhatha vibhatha commented May 13, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

Adding RangeEqualsVisitor for StringView as discussed in #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

vibhatha
vibhatha commented May 13, 2024
View reviewed changes
Comment thread java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 13, 2024
@vibhatha
vibhatha marked this pull request as ready for review May 13, 2024 10:22
@vibhatha
vibhatha requested a review from lidavidm as a code owner May 13, 2024 10:22
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels May 13, 2024
lidavidm
lidavidm reviewed May 14, 2024
View reviewed changes
Comment thread java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java Outdated
Comment thread java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java Outdated
Comment thread java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java Outdated
Comment thread java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java Outdated

@lidavidm lidavidm May 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also test nulls, and ranges that should not be equal

@vibhatha vibhatha May 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one case for that.

@vibhatha
vibhatha force-pushed the stringview-range-equals-visitor branch from d8d140f to ec55b8b Compare May 14, 2024 23:57
@vibhatha
vibhatha requested a review from lidavidm May 14, 2024 23:58
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 14, 2024
lidavidm
lidavidm requested changes May 15, 2024
View reviewed changes
Comment thread java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java
Comment on lines +154 to +155
// unequal range
assertTrue(visitor.rangeEquals(new Range(8, 0, 3)));

@lidavidm lidavidm May 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

....they're still equal. Let's test ranges that are not supposed to be equal.

@vibhatha vibhatha May 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right! 🙂 I misunderstood.

@vibhatha vibhatha May 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vibhatha vibhatha May 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added null cases, there is a bit of code duplication in testing the ranges, I left it that way for the readability.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 15, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 15, 2024
@vibhatha

vibhatha commented May 15, 2024

Copy link
Copy Markdown
Contributor Author

I am working on adding the null one...

vibhatha
vibhatha commented May 15, 2024
View reviewed changes
Comment thread java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java Outdated
return false;
}

if (!isNull) {

@vibhatha vibhatha May 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidavidm there was an unnecessary looping happened. So I added this. Sorry I missed this earlier.

@lidavidm lidavidm May 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's clearer to do if (isNull) { continue; }

lidavidm
lidavidm approved these changes May 15, 2024
View reviewed changes
Comment thread java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java Outdated
return false;
}

if (!isNull) {

@lidavidm lidavidm May 15, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's clearer to do if (isNull) { continue; }

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 15, 2024
@lidavidm
lidavidm merged commit 1c15c88 into apache:main May 16, 2024
@lidavidm lidavidm removed the awaiting merge Awaiting merge label May 16, 2024
@lidavidm lidavidm mentioned this pull request May 16, 2024
Closed
@conbench-apache-arrow

conbench-apache-arrow Bot commented May 16, 2024

Copy link
Copy Markdown

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.

vibhatha added a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@vibhatha
apacheGH-40943: [Java] Implement RangeEqualsVisitor for StringView (a…
2e1cf22 
…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>
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@vibhatha @kou
apacheGH-40943: [Java] Implement RangeEqualsVisitor for StringView (a…
53b71cf 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes

Assignees

No one assigned

Labels

Component: Java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vibhatha @lidavidm