ARROW-11628: [Rust] Made Bytes typed
#9496
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9496 +/- ##
==========================================
- Coverage 82.12% 82.06% -0.06%
==========================================
Files 235 236 +1
Lines 54729 54767 +38
==========================================
+ Hits 44944 44945 +1
- Misses 9785 9822 +37
Continue to review full report at Codecov.
|
rust/arrow/src/bytes.rs
Outdated
There was a problem hiding this comment.
Is there a reason to change from // to ///?
| pub struct Buffer { | ||
| /// the internal byte buffer. | ||
| data: Arc<Bytes>, | ||
| data: Arc<Bytes<u8>>, |
There was a problem hiding this comment.
@jorgecarleitao if Buffer always has Bytes<u8>, I think this alone doesn't address one of the motivations of doing this; being that we'd like to know what boundary to slice buffers in when offsetting arrays.
This would be when using Buffer::offset and Buffer::length on nested arrays.
There was a problem hiding this comment.
I agree.
However, we can't make Buffer<T> because ArrayData force us to use an homogeneous type across all buffers. We can't drop ArrayData because FFI, IPC, CSV, JSON, equality and array/transform depend on it.
This PR was a stop-gap.
Th target design for me is in the proposal. Specifically, for primitive arrays, it would look like this
But because of what I wrote (a lot of dependencies on ArrayData), it requires a large (like +10k lines large) refactor of this crate.
There was a problem hiding this comment.
Is it worth still pursuing adding more information to Buffer while working separately on the proposal? I'm blocked on some parquet work that I'm doing, because it depends on being able to correctly slice nested arrays.
There was a problem hiding this comment.
I think that any work that unblocks you is worth pursuing :)
A path could be:
- Make
Buffersupport both bit slices and byte slices:
- Add a new
offset_bitstoBuffer. - Expose a method
Buffer::slice_bitmapthat incrementsoffset_bits(and panics if called withoffset != 0). - Make
Buffer::slicepanic if called withoffset_bits != 0.
These will allow us to slice Buffer when it is either holder of bits and a holder of bytes (we just need to use either API independently). This is required because the Boolean array's values are in bits (and thus part of ArrayData::buffers).
- Implement a method
pub fn slice(&self, offset: usize, length: usize) -> Self
on every array, that calls slice or slice_bits depending on what the Buffer represents (and also fix the incorrect slicing of arrays with offsets).
-
make
Array::slicecall the concrete implementations -
Make all calls of
get_bitand the like to useBuffer::offset_bitsto compute slot nullability and/or values.
Note that I have not tested this and thus this may not work. I also do not know the consequences to IPC and FFI. :/
I am more confident that the proposal works because it passes IPC tests (IPC read and write, little and big endian ;)).
|
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories arrow-rs and arrow-datafusion. It is likely we will not merge this PR into this repository Please see the mailing-list thread for more details We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs. |
4 participants
This PR makes
Bytestyped.Bytesis not a public API and therefore this is backward compatible.Same rational and built on top of #9495