-
Notifications
You must be signed in to change notification settings - Fork 875
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
Support FixedSizeBinary
in Row format
#3182
Support FixedSizeBinary
in Row format
#3182
Conversation
da7b20e
to
93c1937
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Shall we add a mention of fixed length byte encoding in the docs:
Lines 215 to 261 in 6f41b95
/// ## Variable Length Bytes (including Strings) Encoding | |
/// | |
/// A null is encoded as a `0_u8`. | |
/// | |
/// An empty byte array is encoded as `1_u8`. | |
/// | |
/// A non-null, non-empty byte array is encoded as `2_u8` followed by the byte array | |
/// encoded using a block based scheme described below. | |
/// | |
/// The byte array is broken up into 32-byte blocks, each block is written in turn | |
/// to the output, followed by `0xFF_u8`. The final block is padded to 32-bytes | |
/// with `0_u8` and written to the output, followed by the un-padded length in bytes | |
/// of this final block as a `u8`. | |
/// | |
/// Note the following example encodings use a block size of 4 bytes, | |
/// as opposed to 32 bytes for brevity: | |
/// | |
/// ```text | |
/// ┌───┬───┬───┬───┬───┬───┐ | |
/// "MEEP" │02 │'M'│'E'│'E'│'P'│04 │ | |
/// └───┴───┴───┴───┴───┴───┘ | |
/// | |
/// ┌───┐ | |
/// "" │01 | | |
/// └───┘ | |
/// | |
/// NULL ┌───┐ | |
/// │00 │ | |
/// └───┘ | |
/// | |
/// "Defenestration" ┌───┬───┬───┬───┬───┬───┐ | |
/// │02 │'D'│'e'│'f'│'e'│FF │ | |
/// └───┼───┼───┼───┼───┼───┤ | |
/// │'n'│'e'│'s'│'t'│FF │ | |
/// ├───┼───┼───┼───┼───┤ | |
/// │'r'│'a'│'t'│'r'│FF │ | |
/// ├───┼───┼───┼───┼───┤ | |
/// │'a'│'t'│'i'│'o'│FF │ | |
/// ├───┼───┼───┼───┼───┤ | |
/// │'n'│00 │00 │00 │01 │ | |
/// └───┴───┴───┴───┴───┘ | |
/// ``` | |
/// | |
/// This approach is loosely inspired by [COBS] encoding, and chosen over more traditional | |
/// [byte stuffing] as it is more amenable to vectorisation, in particular AVX-256. | |
/// | |
/// ## Dictionary Encoding |
I realize the actual encoding is straightforward
let end_offset = *offset + len + 1; | ||
if let Some(val) = maybe_val { | ||
let to_write = &mut out.buffer[*offset..end_offset]; | ||
to_write[0] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is worth noting that this value is chosen to be neither 0 or 0xff (the null sentinels)?
arrow/src/row/mod.rs
Outdated
@@ -809,6 +809,10 @@ fn new_empty_rows( | |||
.iter() | |||
.zip(lengths.iter_mut()) | |||
.for_each(|(slice, length)| *length += variable::encoded_len(slice)), | |||
DataType::FixedSizeBinary(len) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked and there is no such thing as FixedSizeLargeBinary
arrow-rs/arrow-schema/src/datatype.rs
Lines 155 to 157 in 6f41b95
FixedSizeBinary(i32), | |
/// Opaque binary data of variable length and 64-bit offsets. | |
LargeBinary, |
FixedSizeBinary
in Row format
Benchmark runs are scheduled for baseline = 733d32e and contender = ab3f384. ab3f384 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #.
Rationale for this change
We claim support for this data type, with the implementation actually panicking on
unreachable
if given such an array. It is fairly trivial to add support for this, so lets do soWhat changes are included in this PR?
Are there any user-facing changes?