Skip to content

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

Merged
lidavidm merged 7 commits into
apache:mainfrom
vibhatha:stringview-copy
May 25, 2024
Merged

GH-40933: [Java] Enhance the copyFrom* functionality in StringView#41752
lidavidm merged 7 commits into
apache:mainfrom
vibhatha:stringview-copy

Conversation

@vibhatha

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

Copy link
Copy Markdown
Contributor

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

vibhatha commented May 21, 2024

Copy link
Copy Markdown
Contributor Author

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

@lidavidm

lidavidm commented May 21, 2024

Copy link
Copy Markdown
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

vibhatha commented May 22, 2024

Copy link
Copy Markdown
Contributor Author

@lidavidm this PR is ready for review.

lidavidm
lidavidm requested changes May 22, 2024
View reviewed changes
Comment thread java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated
Comment on lines 1558 to 1565

@lidavidm lidavidm May 22, 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.

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

@vibhatha vibhatha May 22, 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.

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

Comment thread java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java Outdated
@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
Comment thread java/vector/src/main/java/org/apache/arrow/vector/types/Types.java Outdated

@vibhatha vibhatha May 22, 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.

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
Comment thread java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated

@vibhatha vibhatha May 22, 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 If we take instances, then we have a bit of a hard time deallocating memory. Rather I used the types, is this okay?

@lidavidm lidavidm May 23, 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.

Just use factory functions?

@vibhatha vibhatha May 23, 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 updated. Is it as expected?

@lidavidm lidavidm May 24, 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.

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

@vibhatha vibhatha May 24, 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 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
Comment thread java/vector/src/test/java/org/apache/arrow/vector/TestVarCharViewVector.java Outdated

@lidavidm lidavidm May 24, 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.

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.

@vibhatha vibhatha May 24, 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 updated.

@vibhatha vibhatha May 24, 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.

Resolving conflicts...

@vibhatha vibhatha May 24, 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.

Done!

@vibhatha
vibhatha requested a review from lidavidm May 24, 2024 08:14
@lidavidm
lidavidm merged commit 54dfb82 into apache:main May 25, 2024
@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 Bot commented May 25, 2024

Copy link
Copy Markdown

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.

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@vibhatha @kou
apacheGH-40933: [Java] Enhance the copyFrom* functionality in StringV…
d2957ef 
…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>
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