GH-40038: [Java] Export non empty offset buffer for variable-size layout through C Data Interface #40043

viirya · 2024-02-12T08:02:00Z

Rationale for this change

We encountered an error when exchanging string array from Java to Rust through Arrow C data interface. At Rust side, it complains that the buffer at position 1 (offset buffer) is null. After tracing down and some debugging, it looks like the issue is Java Arrow BaseVariableWidthVector class assigns an empty offset buffer if the array is empty (value count 0).

According to Arrow spec for variable size binary layout:

The offsets buffer contains length + 1 signed integers ...

So for an empty string array, its offset buffer should be a buffer with one element (generally it is 0).

What changes are included in this PR?

This patch replaces current empty offset buffer in variable-size layout vector classes when exporting arrays through C Data Interface.

Are these changes tested?

Added test cases.

Are there any user-facing changes?

No

Closes: [Java] The offset buffer of empty BaseVariableWidthVector should not be empty when exposed through C Data Interface #40038

…e-size layout should not be empty

github-actions · 2024-02-12T08:02:29Z

⚠️ GitHub issue #40038 has been automatically assigned in GitHub to PR creator.

viirya · 2024-02-12T08:04:20Z

java/vector/src/main/java/org/apache/arrow/vector/complex/AbstractStructVector.java

+    if (conflictPolicy == ConflictPolicy.CONFLICT_REPLACE && vectors.containsKey(childName)) {
+      vectors.getAll(childName).forEach(c -> c.close());
+    }


This address test failure in TestStructVector. When duplicate field names exist in initializeChildrenFromFields call, the initial offset buffer in previous vectors will cause memory leak. So these duplicate vectors are closed here.

Shouldn't this be done in putVector instead?

viirya · 2024-02-12T08:06:23Z

java/vector/src/main/java/org/apache/arrow/vector/complex/BaseRepeatedValueVector.java

+    if (!init) {
+      offsetAllocationSizeInBytes = curSize;
+    }


This is to avoid changing offsetAllocationSizeInBytes to the initial 4 byte size. Otherwise, reallocOffsetBuffer will take it and cause some tests failed.

viirya · 2024-02-12T08:07:06Z

java/adapter/jdbc/src/test/java/org/apache/arrow/adapter/jdbc/ResultSetUtilityTest.java

@@ -50,6 +50,7 @@ public void testZeroRowResultSet() throws Exception {
        assertNotNull("VectorSchemaRoot from first next() result should never be null", root);
        assertEquals("VectorSchemaRoot from empty ResultSet should have zero rows", 0, root.getRowCount());
        assertFalse("hasNext() should return false on empty ResultSets after initial next() call", iter.hasNext());
+        root.close();


This is needed to release initial offset buffer for empty array.

Perhaps we can use a try instead of calling close explicitly?

viirya

I've run vector tests locally and they are passed. The tests in other modules might still have failure. If any, I will look at them later.

lidavidm

Java is correct here.

https://lists.apache.org/thread/w7g1zfqrjxx0bvrct0mt5zwxvdnc9nob

viirya · 2024-02-12T15:51:08Z

Ah, okay, I see. I've not found the ticket. Looks like both cases (empty or single zero element) are acceptable. I will close this. Thanks for the info.

lidavidm · 2024-02-12T16:29:33Z

Are you using arrow2? It's possible it was never updated to fix this, but arrow-rs appears to have been fixed

viirya · 2024-02-12T16:39:05Z

I'm using arrow-rs. Although its array data allows empty offset, the issue happens in ffi module which doesn't allow empty offsets for now. I proposed a fix there.

lidavidm · 2024-02-12T16:42:43Z

Hmm. While we allow that for IPC, I think I'm wrong about C Data: we explicitly specify this is not allowable https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowArray.buffers

@pitrou presumably Java should actually be fixed here, but what do other implementations do/we appear to be missing tests for this case?

viirya · 2024-02-12T16:46:09Z

The buffer pointers MAY be null only in two situations:
for the null bitmap buffer, if ArrowArray.null_count is 0;
for any buffer, if the size in bytes of the corresponding buffer would be 0.

Isn't it allowed for second situation?

lidavidm · 2024-02-12T17:05:07Z

Ah, fair. It reads as a bit ambiguous to me: would the size of the buffer be 0, or is it that it should not be 0, but we allowed 0 as an exception before? I guess both interpretations lead to the buffer being 0-sized, though, and so it applies. Maybe we should spell out the case...

Regardless, it does seem we're missing an integration test then.

pitrou · 2024-02-12T17:05:40Z

@pitrou presumably Java should actually be fixed here, but what do other implementations do/we appear to be missing tests for this case?

Yes, I think it should be fixed. Zero-length columns are tested in the integration test, but whether they actually miss an offsets buffer depends on how the JSON reader behaves.

lidavidm · 2024-02-12T17:10:06Z

Sorry, this is for the C Data integration tests

lidavidm · 2024-02-12T17:10:18Z

Ah ok, now I see what you mean

pitrou · 2024-02-12T17:03:30Z

java/adapter/jdbc/src/test/java/org/apache/arrow/adapter/jdbc/ResultSetUtilityTest.java

@@ -50,6 +50,7 @@ public void testZeroRowResultSet() throws Exception {
        assertNotNull("VectorSchemaRoot from first next() result should never be null", root);
        assertEquals("VectorSchemaRoot from empty ResultSet should have zero rows", 0, root.getRowCount());
        assertFalse("hasNext() should return false on empty ResultSets after initial next() call", iter.hasNext());
+        root.close();


Perhaps we can use a try instead of calling close explicitly?

java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestDictionaryUtils.java

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

pitrou · 2024-02-12T17:13:21Z

java/vector/src/main/java/org/apache/arrow/vector/complex/AbstractStructVector.java

+    if (conflictPolicy == ConflictPolicy.CONFLICT_REPLACE && vectors.containsKey(childName)) {
+      vectors.getAll(childName).forEach(c -> c.close());
+    }


Shouldn't this be done in putVector instead?

pitrou · 2024-02-12T17:14:59Z

It seems this fix might be a bit heavy-handed if it adds a heap allocation for every empty var-width vector? AFAIU, Arrow Java uses empty vectors quite liberally.

viirya · 2024-02-12T17:16:17Z

Ah, fair. It reads as a bit ambiguous to me: would the size of the buffer be 0, or is it that it should not be 0, but we allowed 0 as an exception before? I guess both interpretations lead to the buffer being 0-sized, though, and so it applies. Maybe we should spell out the case...

I think it should mean that the buffer size is 0, the buffer pointers may be null, for the second situation.

pitrou · 2024-02-12T17:19:04Z

I think it should mean that the buffer size is 0, the buffer pointers may be null, for the second situation.

Yes, but it's the buffer size, not the array size. For an empty array, the buffer size should be 4 (or 8 for a large offsets type), so the buffer can't be null.

viirya · 2024-02-12T17:19:05Z

It seems this fix might be a bit heavy-handed if it adds a heap allocation for every empty var-width vector? AFAIU, Arrow Java uses empty vectors quite liberally.

Yea, and I believe that @lidavidm meant that these empty offsets for empty var-width vector are valid. That's why I closed this and proposed a fix at ffi module at Rust instead.

viirya · 2024-02-12T17:22:36Z

Yes, but it's the buffer size, not the array size. For an empty array, the buffer size should be 4 (or 8 for a large offsets type), so the buffer can't be null.

That's what I thought at the beginning and the reason I proposed this fix.

But somehow from the context @lidavidm provided:
https://lists.apache.org/thread/w7g1zfqrjxx0bvrct0mt5zwxvdnc9nob

I think he means that it is a valid case (empty offset).

viirya · 2024-02-12T17:28:39Z

I guess that this might be something we really need to fix (at least for the C data export part for var-size arrays). Reopened it.

viirya · 2024-03-26T23:42:24Z

cc @sunchao for another eyes on this. Thanks.

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

andygrove · 2024-04-02T15:19:37Z

Thanks for the review @lidavidm

viirya · 2024-04-02T16:26:08Z

Thank you @lidavidm @pitrou @vibhatha @andygrove for the review

conbench-apache-arrow · 2024-04-03T04:25:19Z

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

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.

…ze layout through C Data Interface (apache#40043) ### Rationale for this change We encountered an error when exchanging string array from Java to Rust through Arrow C data interface. At Rust side, it complains that the buffer at position 1 (offset buffer) is null. After tracing down and some debugging, it looks like the issue is Java Arrow `BaseVariableWidthVector` class assigns an empty offset buffer if the array is empty (value count 0). According to Arrow [spec](https://arrow.apache.org/docs/format/Columnar.html#variable-size-binary-layout) for variable size binary layout: > The offsets buffer contains length + 1 signed integers ... So for an empty string array, its offset buffer should be a buffer with one element (generally it is `0`). ### What changes are included in this PR? This patch replaces current empty offset buffer in variable-size layout vector classes when exporting arrays through C Data Interface. ### Are these changes tested? Added test cases. ### Are there any user-facing changes? No * Closes: apache#40038 Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: David Li <[email protected]>

apacheGH-40038: [Java] The offset buffer of empty vector with variabl…

5eb34e1

…e-size layout should not be empty

viirya requested a review from lidavidm as a code owner February 12, 2024 08:02

github-actions bot added Component: Java awaiting committer review Awaiting committer review labels Feb 12, 2024

viirya commented Feb 12, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 12, 2024

viirya commented Feb 12, 2024

View reviewed changes

lidavidm requested changes Feb 12, 2024

View reviewed changes

viirya closed this Feb 12, 2024

viirya mentioned this pull request Feb 12, 2024

Make ffi consume variable layout arrays with empty offsets apache/arrow-rs#5391

Closed

pitrou reviewed Feb 12, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 3, 2024

viirya force-pushed the fix_variable_layout3 branch from 570cdca to aca7403 Compare March 3, 2024 18:56

Try to add test for refcount

0d004c4

viirya force-pushed the fix_variable_layout3 branch from aca7403 to 0d004c4 Compare March 3, 2024 19:11

Check allocated memory

aae9bcd

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 3, 2024

andygrove reviewed Mar 30, 2024

View reviewed changes

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

Add comments

7f4343c

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Mar 30, 2024

advancedxy mentioned this pull request Apr 1, 2024

build: Use specified branch of arrow-rs with workaround to invalid offset buffers from Java Arrow apache/datafusion-comet#239

Merged

lidavidm approved these changes Apr 2, 2024

View reviewed changes

lidavidm merged commit 5ddef63 into apache:main Apr 2, 2024
18 checks passed

lidavidm removed the awaiting changes Awaiting changes label Apr 2, 2024

github-actions bot added the awaiting merge Awaiting merge label Apr 2, 2024

This was referenced May 9, 2024

[Java] The offset buffer of empty BaseVariableWidthVector is not initiated before exporting #41609

Closed

GH-41609: [Java] Initialize non empty offset buffer for variable-size layout before exporting #41610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-40038: [Java] Export non empty offset buffer for variable-size layout through C Data Interface #40043

GH-40038: [Java] Export non empty offset buffer for variable-size layout through C Data Interface #40043

viirya commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024

viirya Feb 12, 2024

pitrou Feb 12, 2024

viirya Feb 12, 2024

viirya Feb 12, 2024

pitrou Feb 12, 2024

viirya left a comment

lidavidm left a comment

viirya commented Feb 12, 2024

lidavidm commented Feb 12, 2024

viirya commented Feb 12, 2024 •

edited

Loading

lidavidm commented Feb 12, 2024

viirya commented Feb 12, 2024

lidavidm commented Feb 12, 2024

pitrou commented Feb 12, 2024

lidavidm commented Feb 12, 2024

lidavidm commented Feb 12, 2024

pitrou Feb 12, 2024

pitrou Feb 12, 2024

pitrou commented Feb 12, 2024

viirya commented Feb 12, 2024

pitrou commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Mar 26, 2024

andygrove commented Apr 2, 2024

viirya commented Apr 2, 2024

conbench-apache-arrow bot commented Apr 3, 2024

GH-40038: [Java] Export non empty offset buffer for variable-size layout through C Data Interface #40043

GH-40038: [Java] Export non empty offset buffer for variable-size layout through C Data Interface #40043

Conversation

viirya commented Feb 12, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Feb 12, 2024

viirya Feb 12, 2024

Choose a reason for hiding this comment

pitrou Feb 12, 2024

Choose a reason for hiding this comment

viirya Feb 12, 2024

Choose a reason for hiding this comment

viirya Feb 12, 2024

Choose a reason for hiding this comment

pitrou Feb 12, 2024

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

lidavidm left a comment

Choose a reason for hiding this comment

viirya commented Feb 12, 2024

lidavidm commented Feb 12, 2024

viirya commented Feb 12, 2024 • edited Loading

lidavidm commented Feb 12, 2024

viirya commented Feb 12, 2024

lidavidm commented Feb 12, 2024

pitrou commented Feb 12, 2024

lidavidm commented Feb 12, 2024

lidavidm commented Feb 12, 2024

pitrou Feb 12, 2024

Choose a reason for hiding this comment

pitrou Feb 12, 2024

Choose a reason for hiding this comment

pitrou commented Feb 12, 2024

viirya commented Feb 12, 2024

pitrou commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Feb 12, 2024

viirya commented Mar 26, 2024

andygrove commented Apr 2, 2024

viirya commented Apr 2, 2024

conbench-apache-arrow bot commented Apr 3, 2024

viirya commented Feb 12, 2024 •

edited

Loading

viirya commented Feb 12, 2024 •

edited

Loading