geoarrow
Adds Take Impl for GeometryArray
- Implements the Take trait for GeometryArray
- Adds GeometryArray as a valid type for NativeArray's Take Impl
closes #985
Thanks for the PR!
Since originally implementing the Take here, I learned about the upstream take, which is doing basically the same thing as we do here.
I'm happy to merge this provisionally, but I'm thinking that we may either want to remove our own Take implementation or change it to be just a wrapper around the upstream arrow::compute::take, where our wrapper maintains the extension type information.
What do you think?
I think relying on the upstream's take in some way is a great approach. My initial thought is that providing a wrapper that preserves the type information should be a relatively low lift from a code/maintenance standpoint, but it provides better ergonomics when using geoarrow. I suppose as of right now there's nothing inhibiting users from leveraging the upstream implementation and performing the type conversion on their own (I didn't even think to use that approach before authoring this PR). I can also see a good argument that geoarrow should not be duplicating functionality in arrow::compute because that can lead to disparate APIs like we already have here.
I'd be happy to wholesale refactor the take implementation to use upstream's take, or to remove it altogether
I think the main reason why I originally wanted to have a custom take was in order to select rows from multiple separate batches. But I don't think that's worth the complexity at this point.
So the entire implementation for any geoarrow type that implements both ArrayBase and TryFrom<(ArrayRef, Dimension)> ends up just being a few lines for each type. However, GeometryArray is still a bit of an odd case since it's mixed Dimension (this makes the take implementation a bit shorter actually, since it doesn't need a Dimension and can just directly use try_into() for the return type). Given the simplicity of the solution I could really be convinced either direction here. I do see some value in building the simple wrapper to abstract away the Dimension component from users, if for no other reason than to reduce repeated code that has to handle that for each call to arrow::compute.
I think it's best to point users who want Take to use the upstream arrow-rs implementation directly. I don't think it's worth the complexity to implement it in geoarrow-rs as well.