Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
maksimyego-db opened this issue Oct 9, 2024 · 5 comments
Closed

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

maksimyego-db opened this issue Oct 9, 2024 · 5 comments

Comments

@maksimyego-db
Copy link

maksimyego-db commented Oct 9, 2024

Describe the bug, including details regarding any error messages, version, and platform.

When slicing a VectorSchemaRoot wrapping a NullVector, NullVector.getAllocator() will throw UnsupportedOperationException("Tried to get allocator from NullVector")

It appears that the call could pass in a null for allocator in https://github.com/apache/arrow/blob/release-17.0.0-rc2/java/vector/src/main/java/org/apache/arrow/vector/NullVector.java#L172-L174. Would it make sense to return null instead of throw from NullVector.getAllocator()?

Tangentially related to #30866 that was fixed in #41066 for some vector types.

Component(s)

Java

@vibhatha
Copy link
Collaborator

vibhatha commented Oct 9, 2024

This predates my involvement to Arrow, but I think throw is the right way to go. Because we are requesting an attribute which is defined as something we should not support for a NullVector.

cc @lidavidm

@maksimyego-db
Copy link
Author

maksimyego-db commented Oct 9, 2024

Not to lose sight of the original issue: We should be able to slice a VectorSchemaRoot containing NullVectors, like we do for any other ValueVector types. That NullVector does not require a BufferAllocator suggests to me this attribute is optional for any public methods operating on NullVector.

Another alternative is to case match on NullVector in https://github.com/apache/arrow/blob/release-17.0.0-rc2/java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java#L340-L341 but that seems like a one off workaround where conceivably any public ValueVector APIs that require the allocator parameter should work on NullVectors.

@lidavidm
Copy link
Member

I prefer throwing over null since that makes it evident where the failure occurred, but I suppose in context it's preferable over special casing APIs. (And once we have proper @Nullable annotations that should help.) I would be OK returning null so long as this is clearly documented in the base method.

@maksimyego-db maksimyego-db removed their assignment Nov 4, 2024
@myegorov
Copy link
Contributor

myegorov commented Nov 4, 2024

take

myegorov pushed a commit to myegorov/arrow that referenced this issue Nov 4, 2024
myegorov pushed a commit to myegorov/arrow that referenced this issue Nov 5, 2024
myegorov pushed a commit to myegorov/arrow that referenced this issue Nov 5, 2024
…ector

Signed-off-by: Maksim Yegorov <[email protected]>

Code review comment: Add javadoc to getAllocator base method.
myegorov pushed a commit to myegorov/arrow that referenced this issue Nov 5, 2024
…ector

Signed-off-by: Maksim Yegorov <[email protected]>

Code review comment: Add javadoc to getAllocator base method.

mvn spotless:apply linter
myegorov pushed a commit to myegorov/arrow that referenced this issue Nov 5, 2024
…ector

Signed-off-by: Maksim Yegorov <[email protected]>

Code review comment: Add javadoc to getAllocator base method.

mvn spotless:apply linter

retrigger checks
lidavidm pushed a commit that referenced this issue Nov 6, 2024
…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 #44344

### Are these changes tested?

Added unit test that would trigger an UnsupportedOperationException on the legacy path.
* GitHub Issue: #44344

Authored-by: Maksim Yegorov <[email protected]>
Signed-off-by: David Li <[email protected]>
@lidavidm lidavidm added this to the 19.0.0 milestone Nov 6, 2024
@lidavidm
Copy link
Member

lidavidm commented Nov 6, 2024

Issue resolved by pull request 44631
#44631

@lidavidm lidavidm closed this as completed Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants