GH-40942: [Java] Implement C Data Interface for StringView #41967

vibhatha · 2024-06-04T23:45:55Z

Rationale for this change

Recent inclusion of Utf8View and BinaryView support to Java also requires adding C Data interface for integrating it with other systems.

What changes are included in this PR?

Adding core functionality for C Data interface for Utf8View and BinaryView
Adding RoundtripTest
Adding StreamingTest

Are these changes tested?

Yes, with new tests.

Are there any user-facing changes?

No

GitHub Issue: [Java] Implement C Data Interface for StringView #40942

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

java/c/src/test/java/org/apache/arrow/c/StreamTest.java

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

lidavidm

Can we enable the integration tests?

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

java/c/src/test/java/org/apache/arrow/c/StreamTest.java

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

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

vibhatha · 2024-06-07T05:45:48Z

Can we enable the integration tests?

Let me take a look.

vibhatha · 2024-06-07T07:59:24Z

@lidavidm I updated the PR.

vibhatha · 2024-06-07T09:16:18Z

@lidavidm seems like we have to append an empty data buffer too?[1]: https://github.com/apache/arrow/actions/runs/9414397121/job/25933218677?pr=41967#step:6:8512

[1].

arrow/cpp/src/arrow/c/bridge.cc

Line 1742 in a045770

return Status::Invalid("Expected at least 3 buffers for imported type ",

Is this correct according to the spec?

urvishdesai · 2024-06-07T21:29:07Z

java/c/src/main/java/org/apache/arrow/c/Format.java

+      case Utf8View:
+        return "vu";
+      case BinaryView:
+        return "VZ";


AFAIK the BinaryView format string is lowercase "vz"

You're right.

java/c/src/main/java/org/apache/arrow/c/Format.java

urvishdesai

Added some comments regarding the missing buffer at the end that stores variadic buffer lengths.

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

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

vibhatha · 2024-06-09T01:22:29Z

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

@@ -331,7 +360,7 @@ public List<ArrowBuf> visit(ArrowType.Duration type) {
  }

  @Override
-  public List<ArrowBuf> visit(ListView type) {
+  public List<ArrowBuf> visit(ArrowType.ListView type) {


minor change to keep the consistency which was a typo made in a previous PR.

vibhatha · 2024-06-09T03:02:13Z

@lidavidm I enabled CIs please take a look. Also logic was updated. The initial version didn't have the variadic size buffer.

vibhatha · 2024-06-09T03:03:01Z

Added some comments regarding the missing buffer at the end that stores variadic buffer lengths.

@urvishdesai I updated the PR. Could you please take a look?

java/c/src/test/python/integration_tests.py

java/c/src/main/java/org/apache/arrow/c/ArrayExporter.java

lidavidm · 2024-06-10T01:50:47Z

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

@@ -210,9 +210,38 @@ public List<ArrowBuf> visit(ArrowType.Utf8 type) {
    }
  }

+  private List<ArrowBuf> visitVariableWidthView(ArrowType type) {
+    final int viewBufferIndex = 1;
+    try (ArrowBuf view = importFixedBytes(type, viewBufferIndex, BaseVariableWidthViewVector.ELEMENT_SIZE)) {


Why is view being treated specially here? Shouldn't it be the size buffer?

Treated specially means? Sorry I didn't follow.

Why is the view buffer the first one we import? The one we care about is the sizes buffer.

Ah nothing special there, I can change the order. Validity buffer, view buffer, sizes buffer then variadic buffer.
Any issue doing it this way?

In the sense that...you need the sizes buffer to do anything, so import that first, then discard it at the end? Why are we treating the view buffer specially when it's not special here?

I updated, is it okay?

lidavidm · 2024-06-10T01:51:20Z

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java

+      final int variadicBufferReadOffset = 2;
+      try (ArrowBuf variadicSizeBufferPrime = importBuffer(type, variadicSizeBufferIndex,
+          variadicSizeBufferCapacity)) {
+        variadicSizeBufferPrime.getReferenceManager().retain();


And shouldn't we discard the variadic size buffer at the end, since we don't need it anymore?

Hmm, probably yes, let me check.

Mmm... I thought the try-with-resource would be enough?

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

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

java/vector/src/main/java/org/apache/arrow/vector/ipc/JsonFileReader.java

conbench-apache-arrow · 2024-06-21T14:23:05Z

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

There were no benchmark performance regressions. 🎉

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

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

vibhatha force-pushed the stringview-c-data-interface branch 2 times, most recently from 0039b56 to 9d6670b Compare June 6, 2024 08:05

vibhatha commented Jun 6, 2024

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 6, 2024

vibhatha commented Jun 6, 2024

View reviewed changes

java/c/src/test/java/org/apache/arrow/c/StreamTest.java Outdated Show resolved Hide resolved

vibhatha commented Jun 6, 2024

View reviewed changes

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java Outdated Show resolved Hide resolved

vibhatha marked this pull request as ready for review June 6, 2024 08:19

vibhatha requested a review from lidavidm as a code owner June 6, 2024 08:19

vibhatha changed the title ~~GH-40942: [Java] Implement C Data Interface for StringView [WIP]~~ GH-40942: [Java] Implement C Data Interface for StringView Jun 6, 2024

lidavidm reviewed Jun 7, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jun 7, 2024

vibhatha force-pushed the stringview-c-data-interface branch from c48353f to abd244b Compare June 7, 2024 07:58

vibhatha requested a review from lidavidm June 7, 2024 07:59

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 7, 2024

urvishdesai reviewed Jun 7, 2024

View reviewed changes

java/c/src/main/java/org/apache/arrow/c/Format.java Outdated Show resolved Hide resolved

urvishdesai reviewed Jun 7, 2024

View reviewed changes

java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java Outdated Show resolved Hide resolved

java/c/src/main/java/org/apache/arrow/c/BufferImportTypeVisitor.java Outdated Show resolved Hide resolved

vibhatha force-pushed the stringview-c-data-interface branch from 8dd7d7d to d1f70d7 Compare June 9, 2024 01:21

vibhatha commented Jun 9, 2024

View reviewed changes

lidavidm reviewed Jun 10, 2024

View reviewed changes

github-actions bot added the awaiting changes Awaiting changes label Jun 19, 2024

fix: addressing reviews v4

48b05e9

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 19, 2024

lidavidm reviewed Jun 19, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 19, 2024

fix: addressing reviews v5

e8618db

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 19, 2024

vibhatha added 3 commits June 19, 2024 16:34

fix: addressing reviews v6

bcf5da5

fix: addressing reviews v6

3ef4dcc

fix: addressing reviews v7

460630a

vibhatha requested a review from lidavidm June 20, 2024 00:13

lidavidm reviewed Jun 20, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 20, 2024

fix: addressing review v8

d8c45a0

vibhatha requested a review from lidavidm June 20, 2024 08:49

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 20, 2024

fix: minor update

57cb306

lidavidm approved these changes Jun 21, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jun 21, 2024

lidavidm merged commit 3711657 into apache:main Jun 21, 2024
17 checks passed

lidavidm removed the awaiting merge Awaiting merge label Jun 21, 2024

lidavidm mentioned this pull request Jun 21, 2024

[Java] Implement C Data Interface for StringView #40942

Closed

ViggoC mentioned this pull request Sep 26, 2024

GH-44065: [Java] Implement C Data Interface for RunEndEncodedVector #44241

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-40942: [Java] Implement C Data Interface for StringView #41967

GH-40942: [Java] Implement C Data Interface for StringView #41967

vibhatha commented Jun 4, 2024 •

edited

Loading

lidavidm left a comment

vibhatha commented Jun 7, 2024

vibhatha commented Jun 7, 2024

vibhatha commented Jun 7, 2024

urvishdesai Jun 7, 2024

vibhatha Jun 9, 2024

urvishdesai left a comment

vibhatha Jun 9, 2024

vibhatha commented Jun 9, 2024

vibhatha commented Jun 9, 2024

lidavidm Jun 10, 2024

vibhatha Jun 10, 2024

lidavidm Jun 10, 2024

vibhatha Jun 10, 2024

lidavidm Jun 10, 2024

vibhatha Jun 10, 2024

lidavidm Jun 10, 2024

vibhatha Jun 10, 2024

vibhatha Jun 10, 2024

conbench-apache-arrow bot commented Jun 21, 2024

GH-40942: [Java] Implement C Data Interface for StringView #41967

GH-40942: [Java] Implement C Data Interface for StringView #41967

Conversation

vibhatha commented Jun 4, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

lidavidm left a comment

Choose a reason for hiding this comment

vibhatha commented Jun 7, 2024

vibhatha commented Jun 7, 2024

vibhatha commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urvishdesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Jun 9, 2024

vibhatha commented Jun 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jun 21, 2024

vibhatha commented Jun 4, 2024 •

edited

Loading