Optimize take_fixed_size_binary For Predefined Value Lengths#9535
Optimize take_fixed_size_binary For Predefined Value Lengths#9535tobixdev wants to merge 5 commits intoapache:mainfrom
take_fixed_size_binary For Predefined Value Lengths#9535Conversation
|
run benchmark take_kernels |
|
🤖 Hi @tobixdev, thanks for the request (#9535 (comment)). |
|
I assumed that it would not work but it was worth a shot. Would appreciate someone running the benchmark. I saw approximately -80% on my machine. |
|
run benchmark take_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
It's not quite 80% but still significant. Maybe this difference is due to Aarch64 vs. x86 (my PC).
We're also close to the primitive kernel! Parts of the gap could be explained by the entry size (4 vs. 12 bytes). |
|
run benchmark take_kernels |
|
Will run once more to ensure we can reproduce the results. They look good |
|
🤖 |
| } | ||
| } | ||
| let result_buffer = match size_usize { | ||
| 1 => take_fixed_size::<IndexType, 1>(values.values(), indices), |
There was a problem hiding this comment.
as I read this it results in 7 copies of the code which is probably ok here but we do have to be careful in general to avoid too much bloat
There was a problem hiding this comment.
Yes that's definetely a drawback (for compile time and binary size).
|
🤖: Benchmark completed Details
|
🚀 |
arrow-select/src/take.rs
Outdated
| 2 => take_fixed_size::<IndexType, 2>(values.values(), indices), | ||
| 4 => take_fixed_size::<IndexType, 4>(values.values(), indices), | ||
| 8 => take_fixed_size::<IndexType, 8>(values.values(), indices), | ||
| 12 => take_fixed_size::<IndexType, 12>(values.values(), indices), |
There was a problem hiding this comment.
Is 12 a common enough size?
There was a problem hiding this comment.
That's a good question. All of these are only based on a "gut feeling" that these are used in the wild.
I am happy to remove it and change the benchmark (currently 12 is used). But I suppose that's also just a chosen random value.
There was a problem hiding this comment.
What is an example of 12 bytes of data that are stored in a fixed size byte array?
I think 16 is common for UUID.
| &values_buffer[offset..offset + size_usize] | ||
| }) | ||
| }); | ||
| for slice in array_iter { |
There was a problem hiding this comment.
This might be specialized for non-null as well (
There was a problem hiding this comment.
Do you have a pointer on how we could do that? From what I can tell, array_iter will always have a None value due to indices.null_count() > 0. The null buffer of the values array is only considered at the caller.
Maybe I am also thinking into a different direction.
There was a problem hiding this comment.
Oh wait - the None / Some here is from the indices, not the values.
I think we can build the values / nulls separately like we do in other take kernels.
The nulls we can just cheaply (ref) clone from array_iter, this avoids rebuilding the entire null buffer from scratch.
There was a problem hiding this comment.
I think I had it implemented like this before. Unfortunately, ignoring nulls in the loop that looks up the indices from the buffer could trigger a panic for an out-of-range index that is null. For example, when using the indices 0 (valid), 1 (valid), 10 (null) on an array with length 5, the last access would panic even though the index is null.
If you have a suggestion on how I could separate the null checking from the code I am happy to try it out and report the performance result.
| size_usize: usize, | ||
| ) -> Buffer { | ||
| let values_buffer = values.values().as_slice(); | ||
| let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize); |
There was a problem hiding this comment.
This overallocates for the null case (we could use the indices.null_count() if we want to reduce memory for this.
There was a problem hiding this comment.
I think FSB is treated as a primitive array which also allocates for null values so that we can index into the buffer. So I think we need to allocate for null values.
Here is the code for FixedSizeBinaryArray::new_null:
pub fn new_null(size: i32, len: usize) -> Self {
const BITS_IN_A_BYTE: usize = 8;
let capacity_in_bytes = size.to_usize().unwrap().checked_mul(len).unwrap();
Self {
data_type: DataType::FixedSizeBinary(size),
value_data: MutableBuffer::new_null(capacity_in_bytes * BITS_IN_A_BYTE).into(),
nulls: Some(NullBuffer::new_null(len)),
value_length: size,
len,
}
}
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think this looks find to me -- thanks @Dandandan and @tobixdev
My only question is about the 12 byte case. Let us know what you want to do about that one @tobixdev
arrow-select/src/take.rs
Outdated
| 2 => take_fixed_size::<IndexType, 2>(values.values(), indices), | ||
| 4 => take_fixed_size::<IndexType, 4>(values.values(), indices), | ||
| 8 => take_fixed_size::<IndexType, 8>(values.values(), indices), | ||
| 12 => take_fixed_size::<IndexType, 12>(values.values(), indices), |
There was a problem hiding this comment.
What is an example of 12 bytes of data that are stored in a fixed size byte array?
I think 16 is common for UUID.
Thank you for the reviews @Dandandan and @alamb ! Regarding the 12-byte case, I guess here it's just best if we wait for users to come to us with use cases. @Dandandan LMK if you think there is another straightforwards opportunity that I am missing. My Results: |
4 participants
Which issue does this PR close?
Rationale for this change
The
takekernel is very important for many operations (e.g.,HashJoinin DataFusion IIRC). Currently, there is a gap between the performance of the take kernel for primitive arrays (e.g.,DataType::UInt32) and fixed size binary arrays of the same length (e.g.,FixedSizeBinary<4>).In our case this lead to a performance reduction when moving from an integer-based id column to a fixed-size-binary-based id column. This PR aims to address parts of this gap.
The 16-bytes case would especially benefit operations on UUID columns.
What changes are included in this PR?
take_fixed_sizethat can be called for set of predefined fsb-lengths that we want to support. This is a "flat buffer" version of thetake_nativekernel.Are these changes tested?
I've added another test that still exercises the non-optimized code path.
Are there any user-facing changes?
No