We can't implement From as this would conflict with the blanket From<AsRef<[u8]>>

Adding Vec conversion at the Buffer level does allow for transmuting types provided they have the same layout, this is perhaps not ideal but isn't harmful.

Long-term I hope to deprecate and remove Buffer and MutableBuffer, just leaving ScalarBuffer which statically prevents this sort of type-conversion, but doing it this way allows for a gradual migration.

this is perhaps not ideal but isn't harmful.

Is it a problem for something like converting integers to (invalid) floats 🤔

Nope, there is a good justification for why this is perfectly fine here. The only place where this becomes problematic is where types have bit sequences that illegal, e.g. NonZeroU32 or bool, all ArrowNativeType are valid for all bit sequences.

I think it would be good to explicitly say here an error is returned if the buffer can't be converted to a Vec (and ideally hint how to get a Vec out of it anyways (perhaps by copying)

I wonder if the comment for Custom should be updated to say like "external Rust Vec" or maybe remove the reference to Rust Vec entirely

It seems like into_raw_parts would be ideal here, but sadly it appears to not yet be stabalized

https://doc.rust-lang.org/beta/std/vec/struct.Vec.html#method.into_raw_parts

I note that the docs say

After calling this function, the caller is responsible for the memory previously managed by the Vec. The only way to do this is to convert the raw pointer, length, and capacity back into a Vec with the from_raw_parts function, allowing the destructor to perform the cleanup.

However, this code uses Deallocation::Standard(layout) to deallocate memory, which seems ok, I just wanted to point out it doesn't match the docs (though maybe this is fine)

The docs for dealloc are authoritative here

ptr must denote a block of memory currently allocated via this allocator
layout must be the same layout that was used to allocate that block of memory

The layout of arrays, vecs, etc... is fixed and defined by Layout::array. As such we are fine provided we obey the same logic.

Do we plan to store/create from layouts other than Vec? Otherwise we can just create from Vec, forget the owned Vector.

On drop we recreate the Vec so that the drop sequence is executed.

This would defer all this logic to the implementation in std.

Do we plan to store/create from layouts other than Vec? Otherwise we can just create from Vec, forget the owned Vector.

Eventually we may deprecate and remove support for other layouts, but at least for a period we need to support aligned layouts such as those created by MutableBuffer so that we can avoid stop-the-world changes

Right, maybe we could add a comment that once we remove that support we could remove the unsafe layout related code and do something similar to what we do here: https://github.com/DataEngineeringLabs/foreign_vec/blob/0d38968facee8a81748ec380fad78379d806fe1d/src/lib.rs#L25

unsafe layout related code and do something similar to what we do here

What would be the advantage of this? Does polars depend on the foreign vec interface? It doesn't seem to be doing anything materially different?

Edit: In fact I'm fairly sure this line is technically unsound - https://github.com/DataEngineeringLabs/foreign_vec/blob/0d38968facee8a81748ec380fad78379d806fe1d/src/lib.rs#L49, there are a lot of safety constraints on from_raw_parts that appear to be not being checked or documented?

Is there a reason to use unwrap_unchecked here? Maybe it would be better to panic if the layout was messed up somehow?

Looks like it errors on overflow https://doc.rust-lang.org/beta/std/alloc/struct.Layout.html#method.array

Like maybe the size overflows because someone passed in a giant Vec or something?

🤔

Is there a reason to use unwrap_unchecked here

This is a clone of Vec::current_memory, I will add a link.

The TLDR is that if the Vec is valid, it must have a valid layout (otherwise among other issues Vec wouldn't be able to deallocate itself).

this is perhaps not ideal but isn't harmful.

Is it a problem for something like converting integers to (invalid) floats 🤔

I think it is also worth a test when the capacity doesn't equally divide equally into the transmuted size -- like maybe change this test to have capacity 5 with i128 and verify the i256 output Vec only has capacity 2

A capacity of 5 is tested above and results in an error, as the layout of the underlying allocation is invalid

Is slicing with a non zero offset also covered? Maybe slice(2) below covers that 🤔

Yeah slice(2) covers this

maybe it is worth updating these docs to explain when an Err is returned

😍