[Java] Type-ids in UnionVector are erroneously coupled to the Arrow types of the underlying vectors

[jarohen]

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

re: https://lists.apache.org/thread/z89xlvw7v1rwq89gknflhsj3c65x20kd

It seems that the UnionVector implementation (particularly initializeChildrenFromFields (#29848), getVector, getVectorByType, setSafe etc) assumes that the type-id is always based on the ArrowType, but the Schema.fbs spec is more lenient - users have the choice to use whatever type-ids they require.

For example, in XTDB, we're trying to represent an algebraic data type (ADT) of 'put', 'delete' and 'erase' events as a sparse union. Delete and erase have the same type, so UnionVector currently expects them to be the same type-id (whereas, in DenseUnionVector, we can use type-ids 0, 1 and 2).

Would there be an appetite for (potentially relatively significant) changes to UnionVector to make it behave this way? We could perhaps consider bringing it more in line with DenseUnionVector which seems closer to the spec. Would be happy to work on it if so.

Cheers,

James/Finn (@FiV0)

Component(s)

Java

[pitrou]

Would there be an appetite for (potentially relatively significant) changes to UnionVector to make it behave this way? We could perhaps consider bringing it more in line with DenseUnionVector which seems closer to the spec.

+1. It would of course be better if a way of doing this without breaking compatibility with existing Java code is found.

cc @lidavidm @vibhatha @jduo

[lidavidm]

Are you speaking of significant API changes, or significant implementation changes? The latter is fine so long as existing tests pass.

[pitrou]

I'm thinking of significant API changes.

[FiV0]

I think there need to be significant (breaking) API changes if you want to make it spec compliant.

[vibhatha]

+1 for implementation changes preserving compatibility and current test coverage.
Any idea on the maginitude of this change?

[vibhatha]

If this is spec compliant, meaning it changes the existing spec, would it require a vote on ML?

[FiV0]

This may sound harsh, but for me the current UnionVector API is broken in the light of the mailing list discussion above. Even looking at the first test of UnionVector, a line like
https://github.com/apache/arrow/blob/41a989c81616aea103e554521fa6d6209ffa248d/java/vector/src/test/java/org/apache/arrow/vector/TestUnionVector.java#L78
doesn't make sense. You could probably make a lot of the methods that exist work with only internal changes, but I would still argue for deprecation to discourage future use.

Any idea on the maginitude of this change?

A significant rewrite of UnionVector. I might even start from scratch basing it more on the DenseUnionVector implementation.

[pitrou]

Yes, we should definitely deprecate the ill-designed APIs. But it's better if they can still work as originally until people migrate their code to the new APIs.

[vibhatha]

@pitrou I agree with that. Would it be possible to introduce a new API (as required by the discussed change) and leave the older API in deprecate mode and eventually let the migration happen at user end?

[pitrou]

That's what I'm suggesting, yes.

[nbauernfeind]

Are there any plans on fixing this? It's inconvenient that UnionVector#setType and DenseUnionVector#setTypeId deviate from one another.