GH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector #44631

myegorov · 2024-11-04T03:22:26Z

Rationale for this change

Do not throw UnsupportedOperationException("Tried to get allocator from NullVector") from VectorSchemaRoot.slice() when slicing a VSR containing a NullVector or ZeroVector. Details in #44344

Are these changes tested?

Added unit test that would trigger an UnsupportedOperationException on the legacy path.

GitHub Issue: [Java] VectorSchemaRoot.slice() throws for NullVector #44344

myegorov · 2024-11-04T03:26:06Z

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

  @Override
  public BufferAllocator getAllocator() {
-    throw new UnsupportedOperationException("Tried to get allocator from NullVector");
+    return null;


I haven't added a test to check this returns null because this seems to me an implementation detail. Please let me know if you think otherwise.

vibhatha

I assumed we are going to wait till nullability annotations are properly integrated.

myegorov · 2024-11-04T04:21:09Z

I assumed we are going to wait till nullability annotations are properly integrated.

I think I understand the value of requiring @Nullable checks, but wondering if the bugfix here should be blocked on the refactor? iiuc the return null in this PR is far from exceptional.

The ability to make zero-copy slices of ValueVector and VectorSchemaRoot is important to our usage of Arrow. Do you think it's possible to fix the bug and file an issue to forbid null by default as a fast follow? I'd be happy to look into this later but might require some rampup.

vibhatha · 2024-11-04T05:56:25Z

Right. I understand your point. We could create an issue for this and comment in the method.

@lidavidm WDYT?

lidavidm

Can we update the base getAllocator docstring to note that some subclasses may not have an allocator? Otherwise I didn't mean that we should block on nullability annotations, just that it would provide an additional layer of checks.

vibhatha · 2024-11-05T03:12:18Z

Can we update the base getAllocator docstring to note that some subclasses may not have an allocator? Otherwise I didn't mean that we should block on nullability annotations, just that it would provide an additional layer of checks.

Fair enough. Thanks for clarifying @lidavidm

…ector Signed-off-by: Maksim Yegorov <findmaksim@gmail.com> Code review comment: Add javadoc to getAllocator base method. mvn spotless:apply linter retrigger checks

myegorov · 2024-11-05T05:24:41Z

Any idea of what's causing the protoc-gen-grpc-java errors in https://github.com/apache/arrow/actions/runs/11677293979/job/32514967839?

Error: PROTOC FAILED: /build/java/flight/flight-core/target/protoc-plugins/protoc-gen-grpc-java-1.68.1-linux-x86_64.exe: /lib64/libstdc++.so.6: version `CXXABI_1.3.8' not found (required by /build/java/flight/flight-core/target/protoc-plugins/protoc-gen-grpc-java-1.68.1-linux-x86_64.exe)

Seems like a flaky error? #43264 (comment)

lidavidm · 2024-11-05T05:48:07Z

Hmm...I merged a gRPC upgrade earlier which had passed CI but maybe it's not quite safe yet. (I recall some newer version of gRPC or its dependencies started requiring newer glibc)

lidavidm · 2024-11-05T12:14:45Z

Yup, reverting... #44645

conbench-apache-arrow · 2024-11-06T07:40:06Z

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

There were no benchmark performance regressions. 🎉

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

…ector (apache#44631) Do not throw [UnsupportedOperationException("Tried to get allocator from NullVector")](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/NullVector.java#L160) from [VectorSchemaRoot.slice()](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java#L341) when slicing a VSR containing a NullVector or ZeroVector. Details in apache#44344 Added unit test that would trigger an UnsupportedOperationException on the legacy path. * GitHub Issue: apache#44344 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>

…tor (apache#44631) (#94) * apacheGH-44626: [Java] fix SplitAndTransfer throws for empty MapVector (apache#44627) Empty MapVector.splitAndTransfer throws `java.lang.IndexOutOfBoundsException`. Details in apache#44626 Fixed for MapVector as for other vector types in apache#41066 Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0. * GitHub Issue: apache#44626 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> * apacheGH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector (apache#44631) Do not throw [UnsupportedOperationException("Tried to get allocator from NullVector")](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/NullVector.java#L160) from [VectorSchemaRoot.slice()](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java#L341) when slicing a VSR containing a NullVector or ZeroVector. Details in apache#44344 Added unit test that would trigger an UnsupportedOperationException on the legacy path. * GitHub Issue: apache#44344 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com> --------- Signed-off-by: David Li <li.davidm96@gmail.com> Co-authored-by: Maksim Yegorov <997437+myegorov@users.noreply.github.com>

…ector (apache#44631) ### Rationale for this change Do not throw [UnsupportedOperationException("Tried to get allocator from NullVector")](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/NullVector.java#L160) from [VectorSchemaRoot.slice()](https://github.com/apache/arrow/blob/release-18.0.0-rc0/java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java#L341) when slicing a VSR containing a NullVector or ZeroVector. Details in apache#44344 ### Are these changes tested? Added unit test that would trigger an UnsupportedOperationException on the legacy path. * GitHub Issue: apache#44344 Authored-by: Maksim Yegorov <59841139+maksimyego-db@users.noreply.github.com> Signed-off-by: David Li <li.davidm96@gmail.com>

myegorov requested a review from lidavidm as a code owner November 4, 2024 03:22

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

myegorov commented Nov 4, 2024

View reviewed changes

vibhatha reviewed Nov 4, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 4, 2024

lidavidm approved these changes Nov 5, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Nov 5, 2024

vibhatha approved these changes Nov 5, 2024

View reviewed changes

myegorov force-pushed the ARROW-44344 branch 3 times, most recently from 8c4927b to 8c1c1d8 Compare November 5, 2024 03:59

apacheGH-44344: [Java] Fix VectorSchemaRoot.getTransferPair for NullV…

00f7573

…ector Signed-off-by: Maksim Yegorov <findmaksim@gmail.com> Code review comment: Add javadoc to getAllocator base method. mvn spotless:apply linter retrigger checks

myegorov force-pushed the ARROW-44344 branch from 8c1c1d8 to 00f7573 Compare November 5, 2024 05:16

lidavidm approved these changes Nov 5, 2024

View reviewed changes

lidavidm merged commit c3601a9 into apache:main Nov 6, 2024

lidavidm removed the awaiting merge Awaiting merge label Nov 6, 2024

lidavidm mentioned this pull request Nov 6, 2024

[Java] VectorSchemaRoot.slice() throws for NullVector #44344

Closed

myegorov deleted the ARROW-44344 branch November 6, 2024 00:14

lriggs mentioned this pull request Jul 15, 2025

https://github.com/apache/arrow/issues/44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector (https://github.com/apache/arrow/pull/44631) dremio/arrow#94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector #44631

GH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector #44631

Uh oh!

myegorov commented Nov 4, 2024 •

edited by github-actions bot

Loading

Uh oh!

myegorov Nov 4, 2024

Uh oh!

vibhatha left a comment

Uh oh!

myegorov commented Nov 4, 2024 •

edited

Loading

Uh oh!

vibhatha commented Nov 4, 2024

Uh oh!

lidavidm left a comment

Uh oh!

vibhatha commented Nov 5, 2024

Uh oh!

myegorov commented Nov 5, 2024 •

edited

Loading

Uh oh!

lidavidm commented Nov 5, 2024

Uh oh!

lidavidm commented Nov 5, 2024

Uh oh!

conbench-apache-arrow bot commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector #44631

GH-44344: [Java] fix VectorSchemaRoot.getTransferPair for NullVector #44631

Uh oh!

Conversation

myegorov commented Nov 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

Are these changes tested?

Uh oh!

myegorov Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

vibhatha left a comment

Choose a reason for hiding this comment

Uh oh!

myegorov commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vibhatha commented Nov 4, 2024

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

vibhatha commented Nov 5, 2024

Uh oh!

myegorov commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Nov 5, 2024

Uh oh!

lidavidm commented Nov 5, 2024

Uh oh!

conbench-apache-arrow bot commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

myegorov commented Nov 4, 2024 •

edited by github-actions bot

Loading

myegorov commented Nov 4, 2024 •

edited

Loading

myegorov commented Nov 5, 2024 •

edited

Loading