Skip to content

GH-40933: [Java] Enhance the copyFrom* functionality in StringView #41752

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
Merged
merged 7 commits into from
May 25, 2024
Merged

GH-40933: [Java] Enhance the copyFrom* functionality in StringView #41752

merged 7 commits into from
May 25, 2024

Conversation

vibhatha
Copy link
Collaborator

@vibhatha vibhatha commented May 21, 2024 •
edited by github-actions bot
Loading

Rationale for this change

Initial implementation of StringView doesn't contain copy functionality. This PR adds that feature.

What changes are included in this PR?

This PR adds copyFrom and copyFromSafe functions to BaseVariableWidthViewVector.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@vibhatha
Copy link
Collaborator Author

@lidavidm should we also include test cases for ViewVarBinary ? Though the copy functions are implemented in the base class.

@lidavidm
Copy link
Member

We should test both types yes

@vibhatha vibhatha marked this pull request as ready for review May 22, 2024 00:32
@vibhatha vibhatha requested a review from lidavidm as a code owner May 22, 2024 00:32
@vibhatha
Copy link
Collaborator Author

@lidavidm this PR is ready for review.

lidavidm
lidavidm requested changes May 22, 2024
View reviewed changes
java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated
Comment on lines 1558 to 1565
@ParameterizedTest
@MethodSource("vectorProvider")
public void testCopyFromSafeWithNulls(Class<? extends BaseVariableWidthViewVector> vectorClass)
throws NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException {
try (final BaseVariableWidthViewVector vector = vectorClass.getConstructor(String.class, BufferAllocator.class)
.newInstance(EMPTY_SCHEMA_PATH, allocator);
final BaseVariableWidthViewVector vector2 = vectorClass.getConstructor(String.class, BufferAllocator.class)
.newInstance(EMPTY_SCHEMA_PATH, allocator)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid reflection and just parameterize with the instance instead of the class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes definitely. It looks cool, but unreadable at the same time ;)

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 22, 2024
vibhatha
vibhatha commented May 22, 2024
View reviewed changes
java/vector/src/main/java/org/apache/arrow/vector/types/Types.java
@@ -568,7 +568,7 @@ public FieldWriter getNewFieldWriter(ValueVector vector) {
return new VarBinaryWriterImpl((VarBinaryVector) vector);
}
},
VIEWVARBINARY(Binary.INSTANCE) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

typo fixed here.

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 22, 2024
vibhatha
vibhatha commented May 22, 2024
View reviewed changes
java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated
@@ -1451,6 +1455,200 @@ public void testSafeOverwriteLongFromALongerLongString() {
}
}

static Stream<Arguments> vectorTypeAndClassProvider() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lidavidm If we take instances, then we have a bit of a hard time deallocating memory. Rather I used the types, is this okay?

Copy link
Member

Choose a reason for hiding this comment

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

Just use factory functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated. Is it as expected?

Copy link
Member

Choose a reason for hiding this comment

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

No, you want something like Function<BufferAllocator, BaseVariableWidthViewVector>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated. Does it look okay?

@vibhatha vibhatha requested a review from lidavidm May 22, 2024 08:18
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels May 23, 2024
lidavidm
lidavidm reviewed May 24, 2024
View reviewed changes
java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated

@ParameterizedTest
@MethodSource({"vectorTypeAndClassProvider"})
@MethodSource({"vectorTypeProvider"})
Copy link
Member

Choose a reason for hiding this comment

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

No, so: why not just directly have the Function be the parameter? Instead of bouncing through the type? We never use type in the actual tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah right. I updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolving conflicts...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@vibhatha vibhatha requested a review from lidavidm May 24, 2024 08:14
@lidavidm lidavidm merged commit 54dfb82 into apache:main May 25, 2024
16 of 17 checks passed
@lidavidm lidavidm removed the awaiting change review Awaiting change review label May 25, 2024
@lidavidm lidavidm mentioned this pull request May 25, 2024
Closed
@github-actions github-actions bot added the awaiting merge Awaiting merge label May 25, 2024
vibhatha added a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@vibhatha
apacheGH-40933: [Java] Enhance the copyFrom* functionality in StringV…
715c0c6 
…iew (apache#41752)

### Rationale for this change

Initial implementation of StringView doesn't contain `copy` functionality. This PR adds that feature. 

### What changes are included in this PR?

This PR adds `copyFrom` and `copyFromSafe` functions to `BaseVariableWidthViewVector`. 

### Are these changes tested?

Yes

### Are there any user-facing changes?

No
* GitHub Issue: apache#40933

Lead-authored-by: Vibhatha Abeykoon <vibhatha@gmail.com>
Co-authored-by: Vibhatha Lakmal Abeykoon <vibhatha@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
@conbench-apache-arrow conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 11 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

@lidavidm lidavidm lidavidm approved these changes

Assignees
No one assigned
Labels
awaiting merge Awaiting merge Component: Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vibhatha @lidavidm