search

geoarrow / geoarrow-rs

GeoArrow in Rust, Python, and JavaScript (WebAssembly) with vectorized geometry operations
http://geoarrow.org/geoarrow-rs/
Apache License 2.0
258 stars 17 forks source link

`ToArrow` trait that converts to the downcasted arrow-rs array type #239

Open kylebarron opened 1 year ago

kylebarron commented 1 year ago

In https://github.com/geoarrow/geoarrow-rs/pull/237 we removed to_arrow from GeometryArrayTrait to remove the associated type. But it can be annoying to convert from, say, a LineStringArray to an Arc<dyn Array> just to have to downcast back to a GenericListArray. It would be nice to have ToArrow as a separate trait.

If GeometryArrayTrait depends on ToArrow, then into_array_ref can in most cases use a blanket implementation of Arc::new(self.to_arrow()) I think

lewiszlw commented 1 year ago

Do you mean this?

pub trait IntoArrow {
    type ArrowArray;
    fn into_arrow(self) -> Self::ArrowArray;
}

pub trait GeometryArrayTrait: IntoArrow {
    fn into_array_ref(self) -> ArrayRef;
}

This will lead GeometryArrayTrait can't be used in Arc<dyn GeometryArrayTrait>

kylebarron commented 1 year ago

Yes and no, the first trait should exist but not be required by GeometryArrayTrait.

lewiszlw commented 1 year ago

Is it reasonable that moving into_array_ref method from GeometryArrayTrait to IntoArrow trait?

kylebarron commented 1 year ago

I think we should keep anything on GeometryArrayTrait that has high general functionality while still allowing the trait to be object safe. Therefore, I feel strongly that into_array_ref should exist on GeometryArrayTrait. If it were possible for into_arrow to live there too, that would be great, but I think the benefits of being object safe are pretty high