Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-40937: [Java] Implement Holder-based functions for ViewVarCharVector & ViewVarBinaryVector #44187

Merged
merged 8 commits into from
Sep 26, 2024
Merged

GH-40937: [Java] Implement Holder-based functions for ViewVarCharVector & ViewVarBinaryVector #44187

merged 8 commits into from
Sep 26, 2024
+223 −24

Conversation

ViggoC
Copy link
Contributor

@ViggoC ViggoC commented Sep 23, 2024 •
edited
Loading

@ViggoC ViggoC marked this pull request as ready for review September 23, 2024 08:14
@ViggoC ViggoC requested a review from lidavidm as a code owner September 23, 2024 08:14
@ViggoC
Copy link
Contributor Author

ViggoC commented Sep 23, 2024

@vibhatha @lidavidm Could you review this PR?

vibhatha
vibhatha reviewed Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

Added a few comments. Thanks for starting this work.

java/vector/src/main/java/org/apache/arrow/vector/ViewVarCharVector.java Outdated Show resolved Hide resolved
java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated

NullableViewVarCharHolder stringHolder = new NullableViewVarCharHolder();

setAndCheck(viewVarCharVector, 1, strings.get(0), stringHolder);
Copy link
Collaborator

Choose a reason for hiding this comment

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

To check overwrite?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? Could you explain a bit?

Copy link
Contributor Author

@ViggoC ViggoC Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's not needed. I just missed it while adjusting the code. But you reminded me that it is necessary to add some overwrite test cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is a must. It is okay to leave it at this moment.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 23, 2024
vibhatha
vibhatha approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

PR LGTM. Just a few minor suggestions.
Thanks for working on this.

java/vector/src/main/java/org/apache/arrow/vector/ViewVarCharVector.java Outdated Show resolved Hide resolved
java/vector/src/main/java/org/apache/arrow/vector/ViewVarCharVector.java Outdated Show resolved Hide resolved
@vibhatha
Copy link
Collaborator

@lidavidm PR LGTM, please take a look.

@ViggoC
Copy link
Contributor Author

ViggoC commented Sep 25, 2024

PR LGTM. Just a few minor suggestions. Thanks for working on this.

Thanks for your review.

@ViggoC
Copy link
Contributor Author

ViggoC commented Sep 25, 2024 •
edited
Loading

@vibhatha @lidavidm I think the code to support ViewVarBinaryHolder is almost a duplicate of this PR. Do you prefer to fix it in this one or submit another PR.

@vibhatha
Copy link
Collaborator

@ViggoC let's do it here.

@lidavidm
Copy link
Member

Whichever is easier for you.

@ViggoC ViggoC changed the title GH-40937: [Java] Implement Holder-based functions for ViewVarCharVector GH-40937: [Java] Implement Holder-based functions for ViewVarCharVector & ViewVarBinaryVector Sep 25, 2024
vibhatha
vibhatha reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

Also we need the equivalent tests for binary data.

java/vector/src/main/java/org/apache/arrow/vector/ViewVarBinaryVector.java Outdated
Comment on lines 134 to 135
public void get(int index, NullableViewVarBinaryHolder holder) {
// TODO: https://github.com/apache/arrow/issues/40936
throw new UnsupportedOperationException("Unsupported operation");
public void get(int index, NullableViewVarCharHolder holder) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect? Does this compile?

vibhatha
vibhatha reviewed Sep 25, 2024
View reviewed changes
java/vector/src/test/java/org/apache/arrow/vector/TestViewVarBinaryVector.java Outdated
@@ -0,0 +1,144 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend to keep them in the TestVarCharViewVector for the moment. I know the naming is not so matching here. For other VarBinaryView tests we used the same test class. And I see it is not very matching. Maybe we could rename the class to TestVariableWidthViewVector? This is an issue from my part in the initial PR. We can fix it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall PR LGTM, could we just to this change @ViggoC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename the test class to TestVariableWidthViewVector and move testSetNullableViewVarBinaryHolder to it.

vibhatha
vibhatha approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@vibhatha vibhatha left a comment

Choose a reason for hiding this comment

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

Thanks PR LGTM!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 26, 2024
@lidavidm lidavidm merged commit bc923bd into apache:main Sep 26, 2024
17 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Sep 26, 2024
@lidavidm lidavidm mentioned this pull request Sep 26, 2024
Closed
@vibhatha vibhatha mentioned this pull request Sep 26, 2024
Closed
@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit bc923bd.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vibhatha vibhatha vibhatha approved these changes

@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.
3 participants
@ViggoC @vibhatha @lidavidm