Seal Array trait

[tustvold]

Which issue does this PR close?

• Closes Soundness Bug in try_binary when Array is implemented incorrectly in external crate #9106

Rationale for this change

This trait is not meant to be overridden, and doing so will break many kernels in sometimes subtle ways.

What changes are included in this PR?

Seals the Array trait to prevent implementation outside of arrow-array.

Are these changes tested?

Are there any user-facing changes?

[alamb]

Thank you @tustvold @viirya and @Jefffrey

[alamb]

BTW while this is technically an API change and should wait for the next major release, I think it is important enough to merge for 57.2.0 (a minor release) and will not be disruptive (as I don't think anyone has implemented this)

I had a brief look around geoarrow as it is well designed in my opinion and a non trivial extension on top of Arrayss - https://github.com/geoarrow/geoarrow-rs/blob/main/rust/geoarrow-array/src/trait_.rs

I didn't see any impl Array but maybe @kylebarron / @paleolimbot could confirm that that they don't know of any potential downstream issues of making it impossible to impl Array.

[kylebarron]

I tried to implement Array for external types but was never able to get the downcasting working because of the closed nature of DataType. So I don't think it was previously possible to implement Array externally: sealing the array just makes it explicit

[gabotechs]

Such a same getting this trait sealed. I was finding it very convenient for implementing a GPU-based dyn Array baked by https://github.com/rapidsai/cudf in https://github.com/gabotechs/libcudf-rs.

[tustvold]

I presume you're referring to https://github.com/gabotechs/libcudf-rs/blob/main/src/column_view.rs#L125

If so why not just define your own trait, it looks like you're only using a very limited subset of Array anyway, and passing that array into any arrow-rs kernel will cause it at best panic (as downcast won't work correctly).

[gabotechs]

@tustvold It's mainly for wiring it up with https://github.com/apache/datafusion. As DataFusion moves RecordBatches between nodes, which contain dynamic dyn Arrays, that was my means of transportation.

Not saying that the change in this PR does not make sense though, I believe it does, but I wonder what could be the alternative. Maybe letting DataFusion be the one that exposes a customizable trait for transporting data?

[scovich]

I tried to implement Array for external types but was never able to get the downcasting working because of the closed nature of DataType. So I don't think it was previously possible to implement Array externally: sealing the array just makes it explicit

When implementing the variant extension type, we also tried to useArray, but we hit the same 1:1 constraint -- every concrete Array must have a corresponding DataType enum variant in order for downcasting to work properly, and every DataType enum variant must have its own concrete type implementing Array for the same reason. In our case, the (proposed) Array::data_type method for VariantArray had to return DataType::Struct, but attempting to downcast it as StructArray would fail/panic.

So the DataType enum already logically seals the Array trait, but the compiler cannot enforce that. Actually sealing the trait just closes the compiler loophole that currently allows (always provably and unfixably incorrect) third-party implementations of the trait.

Aside: the same 1:1 constrait is also why Array cannot (and must never) require Any, even tho it makes casting a lot harder. Because &dyn Array and Arc<dyn Array> both impl Array (as does &Arc<Arc<dyn Array>> by transitivity) attempting any kind of casting via Array: Any would wreak havoc whenever those convenience wrapper impl are involved.

[scovich]

DataFusion moves RecordBatches between nodes, which contain dynamic dyn Arrays, that was my means of transportation.

Not saying that the change in this PR does not make sense though, I believe it does, but I wonder what could be the alternative. Maybe letting DataFusion be the one that exposes a customizable trait for transporting data?

Could it pass ArrayData instead? Should be easy enough to do e.g. StructArray::from(record_batch).into_data() and then RecordBatch::from(StructArray::from(array_data))?

[alamb]

I am expecting an issue shortly for this PR, and then I think we should merge it to avoid confusion.

I realize this will cause @gabotechs some pain downstream, but let's figure out a better API for backing arrays with GPU memory as a follow on

[gabotechs]

Could it pass ArrayData instead? Should be easy enough to do e.g. StructArray::from(record_batch).into_data() and then RecordBatch::from(StructArray::from(array_data))?

Yeah, I think I can do either this or something similar, thanks for the options! this change makes sense to me 👍

[alamb]

Thanks again for everyone's thoughts. Also thanks to @shinmao for the report

• Soundness Bug in try_binary when Array is implemented incorrectly in external crate #9106

[waynexia]

Another one impl Array for T example here https://github.com/GreptimeTeam/greptimedb/blob/e0285209cb62b398ce89a033a1101991d4a28b27/src/promql/src/range_array.rs#L228 . It wraps a dictionary array into a matrix-like structure.

Despite whether this impl is reasonable, or has a better alternative, Array is one of the biggest abstractions exposed from arrow. Many downstream lib like datafusion only accepts Array (record batch), this sealing also removes a large hack point to play around with data representation and computation. And adding a new implementation of Array to this arrow library is a hard long journey (like variant array) or not even acceptable (like range array for a specific usage). IMHO, this breaking is shipped too fast, and on a minor version.

[alamb]

@waynexia I believe the concern and speed was related to the soundness report here

• Soundness Bug in try_binary when Array is implemented incorrectly in external crate #9106

Given your usecase I believe we should find some other way to sort this out. I filed a ticket to discuss the idea here

• [regression] Sealing the Array trait broke downstream crates #9184

I am happy to run another arrow release on the 57 line to resolve this issue once we have consensus