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

Implement Into<Arc<dyn Array>> for ArrayData #6104

Closed
ParkMyCar opened this issue Jul 23, 2024 · 8 comments
Closed

Implement Into<Arc<dyn Array>> for ArrayData #6104

ParkMyCar opened this issue Jul 23, 2024 · 8 comments
Labels
enhancement Any new improvement worthy of a entry in the changelog

Comments

@ParkMyCar
Copy link

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

At a serialization layer I want to go from ArrayData to an Arc<dyn Array> and some logic higher up will downcast the Arc<dyn Array> into a specific type.

Describe the solution you'd like

Concrete array types, e.g. UInt32Array, implement From<ArrayData>. Implementing Into<Arc<dyn Array>> for ArrayData seems like it would be symmetrical.

Describe alternatives you've considered

The Array trait implements to_data(&self) -> ArrayData, maybe ArrayData could implement to_array(&self) -> Arc<dyn Array>

Additional context

Apologies if this is already possible, I couldn't find it in the docs

@ParkMyCar ParkMyCar added the enhancement Any new improvement worthy of a entry in the changelog label Jul 23, 2024
@alamb
Copy link
Contributor

alamb commented Jul 23, 2024

Seems like a nice improvement to me.

@tustvold
Copy link
Contributor

There is a make_array method that does this

@ParkMyCar
Copy link
Author

Perfect, make_array is what I was looking for! Do you think it would still be worthwhile to implement Into<Arc<dyn Array>> for ArrayData? If not, feel free to close out this issue!

@alamb
Copy link
Contributor

alamb commented Jul 24, 2024

Perfect, make_array is what I was looking for! Do you think it would still be worthwhile to implement Into<Arc<dyn Array>> for ArrayData? If not, feel free to close out this issue!

I think it would be worthwhile as it would make it more discoverable -- could you make a PR @ParkMyCar ?

@tustvold
Copy link
Contributor

I could be mistaken, but I believe the orphan rules may prevent you from doing so

@ParkMyCar
Copy link
Author

Perfect, make_array is what I was looking for! Do you think it would still be worthwhile to implement Into<Arc<dyn Array>> for ArrayData? If not, feel free to close out this issue!

I think it would be worthwhile as it would make it more discoverable -- could you make a PR @ParkMyCar ?

Yeah I'll whip this up! Hopefully orphan rules don't prevent it :)

@ParkMyCar
Copy link
Author

Darn, yeah orphan rules and crate layout currently prevent it.

// approach 1 
//
// In the `arrow-data` crate, this doesn't work because `arrow-array` depends on `arrow-data`
// and we'd end up with a circular dependency.
impl Into<Arc<dyn Array>> for ArrayData { ... }
// approach 2
//
// In the `arrow-array` crate. I thought this would work, and it does if we used
// `Box<dyn Array>`, but `Arc` is not marked as "fundamental" like Box is.
//
// See: https://users.rust-lang.org/t/how-to-bypass-or-relax-orphan-rules/82999
impl From<ArrayData>  for Arc<dyn Array> { ... }

@alamb
Copy link
Contributor

alamb commented Jul 25, 2024

Thanks for trying 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

No branches or pull requests

3 participants