GH-40339: [Java] StringView Initial Implementation #40340

vibhatha · 2024-03-04T13:52:42Z

Rationale for this change

StringView implementation in Java. This PR only includes the core implementation of StringView

What changes are included in this PR?

Adding ViewVarBinaryVector
Adding ViewVarCharVector
Adding corresponding test cases in the given scope
Including required implementation extensions with not supported warnings
Interface for Holders

Non Goals of this PR

Are these changes tested?

Yes. Existing test cases on VarCharVector and VarBinaryVector are verified with view implementations and additional test cases have also been added to check view functionality. And explitly tests have been added to evaluate the view functionality with ViewVarCharVector

Are there any user-facing changes?

Yes, this introduces a new API and some public methods have been included in an interface so that it can be extended to write custom functionality like done for views.

GitHub Issue: [Java] StringView Initial Implementation #40339
GitHub Issue: [Java] StringView Initial Implementation #40339
GitHub Issue: [Java] StringView Initial Implementation #40339
GitHub Issue: [Java] StringView Initial Implementation #40339

lidavidm

Some quick comments. I'm not done looking at this yet.

java/vector/src/main/codegen/data/ValueVectorTypes.tdd

java/vector/src/main/java/org/apache/arrow/vector/AbstractVariableWidthVector.java

java/vector/src/main/java/org/apache/arrow/vector/AddOrGetResult.java

lidavidm · 2024-04-02T10:31:12Z

java/vector/src/main/java/org/apache/arrow/vector/util/Text.java

-    setCapacity(len, false);
-    System.arraycopy(utf8, start, bytes, 0, len);
-    this.length = len;
+    super.set(utf8, start, len);


If we're deferring to super, either this should be @Override or we just shouldn't define this in the first place.

lidavidm · 2024-04-02T10:31:48Z

java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java


+  @Override
+  public Boolean visit(BaseVariableWidthViewVector left, Void value) {
+    throw new UnsupportedOperationException("View vectors are not supported.");


Is this really so difficult to implement that we can't include it here?

It is not the difficulty, I actually thought about the size of this PR. It is already over 3000 LoC. I was merely trying move possible things to separate PRs.

lidavidm · 2024-04-02T10:32:28Z

java/vector/src/main/java/org/apache/arrow/vector/TypeLayout.java

      }

+      @Override
+      public Integer visit(ArrowType.BinaryView type) {


This is going to have to be overhauled, since the buffer count is not fixed anymore.

Yes I agree, I have created an issue for this. How should we proceed?

We need to look at how/where this is used.

At the moment, I kept this unsupported and I already have created an issue to follow up. I am also not sure about this. But if we think about the layout, and keep the growing external buffer as K variable, the base minimum number of buffers can be defined as 2, which is validity and view buffer. But I am not sure if that actually make sense as layout should have that external buffer represented in some way. Or can we include a callback-like thing to get the precise value? But that is again a bit odd as layout supposed to be like an enum-like class.

Not exactly sure what to do here.

// TODO: #40934

vibhatha · 2024-04-02T15:04:25Z

@lidavidm Thanks for the quick comments. I will address these.

lidavidm

@vibhatha can you go over the base implementation again with the spec? It seems to be following the offsets buffer implementation still but none of that applies to view vectors.

java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java

vibhatha · 2024-04-04T06:39:25Z

@vibhatha can you go over the base implementation again with the spec? It seems to be following the offsets buffer implementation still but none of that applies to view vectors.

Sure @lidavidm I will check again.

vibhatha · 2024-04-04T07:11:58Z

@lidavidm the offset usage is incorrect here, it's not needed. I need to fix that. Thanks a lot for catching that.

vibhatha · 2024-04-26T11:29:13Z

@lidavidm I added a few more tests, covering the overwrites for short/long rellocate/no-reallocate with set and with setSafe just simply checking the accuracy since buffer level evaluation would be not very practical for setSafe.

vibhatha · 2024-04-26T11:30:02Z

@github-actions crossbow submit java

github-actions · 2024-04-26T11:32:23Z

Revision: 88d7818

Submitted crossbow builds: ursacomputing/crossbow @ actions-d19dec4add

Task	Status
java-jars
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64

vibhatha · 2024-04-26T12:11:37Z

@lidavidm the java-jars fails, and it seems like the issue is with a C++ failure i.e. arrow-filesystem-test seems to be failing.

vibhatha · 2024-04-26T12:13:51Z

#41371 (You can open a new issue for a CI failure. :-)

@kou it seems to be passing now. Was this fixed in another PR?

kou · 2024-04-26T21:23:44Z

#41379

conbench-apache-arrow · 2024-04-29T04:46:15Z

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

There were no benchmark performance regressions. 🎉

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

### Rationale for this change StringView implementation in Java. This PR only includes the core implementation of StringView ### What changes are included in this PR? - [X] Adding ViewVarBinaryVector - [X] Adding ViewVarCharVector - [X] Adding corresponding test cases in the given scope - [X] Including required implementation extensions with not supported warnings - [X] Interface for Holders ### Non Goals of this PR - [ ] apache#40937 - [ ] apache#40936 - [ ] apache#40932 - [ ] apache#40943 - [ ] apache#40944 - [ ] apache#40942 - [ ] https://github.com/apache/arrow/issues/40945 - [ ] https://github.com/apache/arrow/issues/40941 - [ ] https://github.com/apache/arrow/issues/40946 ### Are these changes tested? Yes. Existing test cases on `VarCharVector` and `VarBinaryVector` are verified with view implementations and additional test cases have also been added to check view functionality. And explitly tests have been added to evaluate the view functionality with `ViewVarCharVector` ### Are there any user-facing changes? Yes, this introduces a new API and some public methods have been included in an interface so that it can be extended to write custom functionality like done for views. * GitHub Issue: apache#40339 Lead-authored-by: Vibhatha Abeykoon <[email protected]> Co-authored-by: vibhatha <[email protected]> Co-authored-by: Vibhatha Lakmal Abeykoon <[email protected]> Signed-off-by: David Li <[email protected]>

github-actions bot added Component: Java awaiting review Awaiting review labels Mar 4, 2024

vibhatha force-pushed the stringview-impl branch from 93cdde7 to 4526423 Compare March 6, 2024 17:43

vibhatha changed the title ~~GH-40339: StringView Initial Implementation for Java~~ GH-40339: [Java] StringView Initial Implementation Mar 12, 2024

vibhatha force-pushed the stringview-impl branch from 5942a63 to 4c36aaf Compare March 28, 2024 00:38

vibhatha force-pushed the stringview-impl branch from daae6dc to 184ed70 Compare April 2, 2024 02:42

This was referenced Apr 2, 2024

[Java] Implement Holder-based functions in ViewVarBinaryVector #40936

Closed

[Java] Implement Holder-based functions for ViewVarCharVector #40937

Closed

vibhatha marked this pull request as ready for review April 2, 2024 10:14

vibhatha requested a review from lidavidm as a code owner April 2, 2024 10:14

vibhatha self-assigned this Apr 2, 2024

lidavidm reviewed Apr 2, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 2, 2024

lidavidm requested changes Apr 4, 2024

View reviewed changes

vibhatha added 13 commits April 26, 2024 16:56

fix: addressing reviews s4 v3.22: format

b7c787b

fix: addressing reviews s4 v3.23: format

bb3bbce

fix: addressing reviews s4 v3.24: format

a40f97c

fix: addressing reviews s4 v3.25: format

a76cb91

fix: updating test names for views

c801042

fix: updating test parameters

be00c88

fix: adding offset check for views

ad80900

fix: adding comments for test variables

203bd7b

fix: updating to junit5

61b9bc1

fix: removing assert usage

89d48a2

fix: adding tests for overwriting v1

586f30e

fix: adding tests for overwriting v2

8770f30

fix: adding tests for overwriting with setSafe

88d7818

vibhatha force-pushed the stringview-impl branch from 4aeb9d2 to 88d7818 Compare April 26, 2024 11:26

lidavidm approved these changes Apr 29, 2024

View reviewed changes

lidavidm merged commit a8c4f86 into apache:main Apr 29, 2024

lidavidm removed the awaiting merge Awaiting merge label Apr 29, 2024

lidavidm mentioned this pull request Apr 29, 2024

[Java] StringView Initial Implementation #40339

Closed

vibhatha mentioned this pull request May 2, 2024

[Java] java-jars fails with "We have duplicated artifacts attached" apache/arrow-java#92

Closed

vibhatha mentioned this pull request Jun 4, 2024

[Java] Implement TransferPair functionality for BinaryView #41968

Closed

GH-40339: [Java] StringView Initial Implementation #40340

GH-40339: [Java] StringView Initial Implementation #40340

Uh oh!

Conversation

vibhatha commented Mar 4, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Non Goals of this PR

Are these changes tested?

Are there any user-facing changes?

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

lidavidm Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

vibhatha Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

lidavidm Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

vibhatha Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

lidavidm Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

vibhatha Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

vibhatha commented Apr 2, 2024

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vibhatha commented Apr 4, 2024

Uh oh!

vibhatha commented Apr 4, 2024

Uh oh!

vibhatha commented Apr 26, 2024

Uh oh!

vibhatha commented Apr 26, 2024

Uh oh!

github-actions bot commented Apr 26, 2024

Uh oh!

vibhatha commented Apr 26, 2024

Uh oh!

vibhatha commented Apr 26, 2024

Uh oh!

kou commented Apr 26, 2024

Uh oh!

conbench-apache-arrow bot commented Apr 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vibhatha commented Mar 4, 2024 •

edited by github-actions bot

Loading