You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tobixdev (via GitHub)" <gi...@apache.org> on 2026/03/11 12:03:55 UTC
[PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
tobixdev opened a new pull request, #9535:
URL: https://github.com/apache/arrow-rs/pull/9535
# Which issue does this PR close?
- Related to https://github.com/apache/arrow-rs/issues/279
# Rationale for this change
The `take` kernel is very important for many operations (e.g., `HashJoin` in 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.
# What changes are included in this PR?
- Add `take_fixed_size` that can be called for set of predefined fsb-lengths that we want to support. This is a "flat buffer" version of the `take_native` kernel.
# Are these changes tested?
I've added another test that still exercises the non-optimized code path.
# Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4038781012
🤖 Hi @tobixdev, thanks for the request (https://github.com/apache/arrow-rs/pull/9535#issuecomment-4038780292). [`scrape_comments.py`](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/scrape_comments.py) only responds to whitelisted users. Allowed users: [Dandandan](https://github.com/Dandandan), [Jefffrey](https://github.com/Jefffrey), [Omega359](https://github.com/Omega359), [adriangb](https://github.com/adriangb), [alamb](https://github.com/alamb), [comphead](https://github.com/comphead), [etseidl](https://github.com/etseidl), [gabotechs](https://github.com/gabotechs), [geoffreyclaude](https://github.com/geoffreyclaude), [klion26](https://github.com/klion26), [rluvaton](https://github.com/rluvaton), [xudong963](https://github.com/xudong963), [zhuqi-lucas](https://github.com/zhuqi-lucas).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2920885212
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
Review Comment:
Yes that's definetely a drawback (for compile time and binary size).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2939123229
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
Review Comment:
That's a good question. All of these are 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4042073315
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group improve-take-fsb main
----- ---------------- ----
take bool 1024 1.00 1327.8±4.95ns ? ?/sec 1.00 1329.2±12.95ns ? ?/sec
take bool 512 1.00 726.9±3.22ns ? ?/sec 1.00 727.1±14.49ns ? ?/sec
take bool null indices 1024 1.00 1097.8±5.99ns ? ?/sec 1.11 1215.4±31.46ns ? ?/sec
take bool null values 1024 1.00 2.6±0.02µs ? ?/sec 1.00 2.6±0.01µs ? ?/sec
take bool null values null indices 1024 1.00 1977.1±21.64ns ? ?/sec 1.48 2.9±0.05µs ? ?/sec
take check bounds i32 1024 1.08 917.4±7.58ns ? ?/sec 1.00 846.9±12.66ns ? ?/sec
take check bounds i32 512 1.00 516.9±21.67ns ? ?/sec 1.14 587.1±1.14ns ? ?/sec
take i32 1024 1.00 712.5±1.52ns ? ?/sec 1.01 717.9±2.41ns ? ?/sec
take i32 512 1.00 441.9±0.76ns ? ?/sec 1.01 444.9±9.14ns ? ?/sec
take i32 null indices 1024 1.00 993.5±2.18ns ? ?/sec 1.00 993.2±2.50ns ? ?/sec
take i32 null values 1024 1.01 2.0±0.00µs ? ?/sec 1.00 2.0±0.00µs ? ?/sec
take i32 null values null indices 1024 1.00 2.1±0.02µs ? ?/sec 1.05 2.2±0.02µs ? ?/sec
take primitive fsb value len: 12, indices: 1024 1.00 1056.9±6.85ns ? ?/sec 3.28 3.5±0.02µs ? ?/sec
take primitive fsb value len: 12, null values, indices: 1024 1.00 2.4±0.04µs ? ?/sec 1.99 4.9±0.01µs ? ?/sec
take primitive run logical len: 1024, physical len: 512, indices: 1024 1.00 21.0±0.24µs ? ?/sec 1.00 21.0±0.10µs ? ?/sec
take str 1024 1.01 11.3±0.09µs ? ?/sec 1.00 11.2±0.11µs ? ?/sec
take str 512 1.00 5.4±0.02µs ? ?/sec 1.02 5.5±0.09µs ? ?/sec
take str null indices 1024 1.00 7.9±0.20µs ? ?/sec 1.00 7.8±0.02µs ? ?/sec
take str null indices 512 1.00 3.8±0.02µs ? ?/sec 1.00 3.8±0.01µs ? ?/sec
take str null values 1024 1.00 8.6±0.13µs ? ?/sec 1.00 8.6±0.03µs ? ?/sec
take str null values null indices 1024 1.00 6.9±0.06µs ? ?/sec 1.02 7.0±0.09µs ? ?/sec
take stringview 1024 1.00 811.9±1.46ns ? ?/sec 1.09 887.2±12.82ns ? ?/sec
take stringview 512 1.00 582.0±7.36ns ? ?/sec 1.01 588.3±2.38ns ? ?/sec
take stringview null indices 1024 1.00 1436.1±22.29ns ? ?/sec 1.01 1449.0±24.56ns ? ?/sec
take stringview null indices 512 1.00 727.5±2.41ns ? ?/sec 1.10 801.0±4.78ns ? ?/sec
take stringview null values 1024 1.00 2.1±0.05µs ? ?/sec 1.00 2.1±0.02µs ? ?/sec
take stringview null values null indices 1024 1.00 2.3±0.02µs ? ?/sec 1.05 2.4±0.03µs ? ?/sec
```
</p>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4042509889
```
take primitive fsb value len: 12, indices: 1024 1.00 1056.9±6.85ns ? ?/sec 3.28 3.5±0.02µs ? ?/sec
take primitive fsb value len: 12, null values, indices: 1024 1.00 2.4±0.04µs ? ?/sec 1.99 4.9±0.01µs ? ?/sec
```
🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2944670897
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
Review Comment:
We could probably make this faster by copying constant-sized arrays instead of "slices", also the builder APIs can have some non-trivial overhead sometimes.
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
Review Comment:
We could probably make this faster by copying constant-sized arrays instead of slices, also the builder APIs can have some non-trivial overhead sometimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2945127690
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
Review Comment:
This overallocates for the null case (we could use the `indices.null_count()` if we want to reduce memory for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2944679537
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
+ }
+ } else {
+ // The indices nullability cannot be ignored here because the values buffer may contain
+ // nulls which should not cause a panic.
+ let array_iter = indices.iter().map(|idx| {
+ idx.map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ })
+ });
+ for slice in array_iter {
Review Comment:
This might be specialized for non-null as well (
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2944846130
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
+ }
+ } else {
+ // The indices nullability cannot be ignored here because the values buffer may contain
+ // nulls which should not cause a panic.
+ let array_iter = indices.iter().map(|idx| {
+ idx.map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ })
+ });
+ for slice in array_iter {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2945127690
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
Review Comment:
This overallocates for the null case (we can use the `indices.null_count()`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4038780292
run benchmark take_kernels
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4041550231
> take primitive fsb value len: 12, indices: 1024 1.00 1057.2±10.66ns ? ?/sec 3.33 3.5±0.17µs ? ?/sec
> take primitive fsb value len: 12, null values, indices: 1024 1.00 2.4±0.03µs ? ?/sec 2.01 4.9±0.08µs ? ?/sec
It's not quite 80% but still significant. Maybe this difference is due to Aarch64 vs. x86 (my PC).
> take i32 1024 1.00 712.3±2.60ns ? ?/sec 1.01 716.9±6.95ns ? ?/sec
We're also close to the primitive kernel! Parts of the gap could be explained by the entry size (4 vs. 12 bytes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4041369764
🤖 `./gh_compare_arrow.sh` [gh_compare_arrow.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing improve-take-fsb (7aee3d92f3902078adebf4484a3a7421e5a8277b) to 33aed330b962d40e6e6b456bc4cd13ec80967f75 [diff](https://github.com/apache/arrow-rs/compare/33aed330b962d40e6e6b456bc4cd13ec80967f75..7aee3d92f3902078adebf4484a3a7421e5a8277b)
BENCH_NAME=take_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=improve-take-fsb
Results will be posted here when complete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4085116284
Thanks again @tobixdev and @Dandandan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2933375858
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
Review Comment:
Is 12 a common enough size?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4041436669
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group improve-take-fsb main
----- ---------------- ----
take bool 1024 1.00 1328.7±5.30ns ? ?/sec 1.00 1328.3±2.79ns ? ?/sec
take bool 512 1.00 728.2±8.58ns ? ?/sec 1.00 727.1±2.58ns ? ?/sec
take bool null indices 1024 1.00 1113.3±13.46ns ? ?/sec 1.11 1232.8±24.14ns ? ?/sec
take bool null values 1024 1.00 2.6±0.02µs ? ?/sec 1.00 2.6±0.01µs ? ?/sec
take bool null values null indices 1024 1.00 2.1±0.07µs ? ?/sec 1.42 2.9±0.04µs ? ?/sec
take check bounds i32 1024 1.10 930.9±68.94ns ? ?/sec 1.00 844.0±4.14ns ? ?/sec
take check bounds i32 512 1.00 515.4±4.77ns ? ?/sec 1.14 588.5±3.92ns ? ?/sec
take i32 1024 1.00 712.3±2.60ns ? ?/sec 1.01 716.9±6.95ns ? ?/sec
take i32 512 1.00 441.8±1.44ns ? ?/sec 1.00 442.8±1.33ns ? ?/sec
take i32 null indices 1024 1.00 995.6±2.39ns ? ?/sec 1.02 1015.2±126.82ns ? ?/sec
take i32 null values 1024 1.01 2.0±0.02µs ? ?/sec 1.00 2.0±0.03µs ? ?/sec
take i32 null values null indices 1024 1.00 2.1±0.02µs ? ?/sec 1.05 2.2±0.08µs ? ?/sec
take primitive fsb value len: 12, indices: 1024 1.00 1057.2±10.66ns ? ?/sec 3.33 3.5±0.17µs ? ?/sec
take primitive fsb value len: 12, null values, indices: 1024 1.00 2.4±0.03µs ? ?/sec 2.01 4.9±0.08µs ? ?/sec
take primitive run logical len: 1024, physical len: 512, indices: 1024 1.00 20.9±0.21µs ? ?/sec 1.00 20.8±0.09µs ? ?/sec
take str 1024 1.00 11.2±0.06µs ? ?/sec 1.01 11.3±0.16µs ? ?/sec
take str 512 1.00 5.4±0.03µs ? ?/sec 1.02 5.5±0.06µs ? ?/sec
take str null indices 1024 1.00 7.9±0.04µs ? ?/sec 1.00 7.9±0.05µs ? ?/sec
take str null indices 512 1.00 3.8±0.01µs ? ?/sec 1.00 3.8±0.03µs ? ?/sec
take str null values 1024 1.00 8.7±0.03µs ? ?/sec 1.00 8.6±0.07µs ? ?/sec
take str null values null indices 1024 1.00 7.0±0.03µs ? ?/sec 1.00 7.0±0.08µs ? ?/sec
take stringview 1024 1.00 809.3±8.35ns ? ?/sec 1.10 893.3±7.40ns ? ?/sec
take stringview 512 1.00 579.9±4.53ns ? ?/sec 1.01 587.5±8.20ns ? ?/sec
take stringview null indices 1024 1.00 1440.7±21.77ns ? ?/sec 1.01 1451.7±4.98ns ? ?/sec
take stringview null indices 512 1.00 718.1±19.57ns ? ?/sec 1.11 800.5±0.89ns ? ?/sec
take stringview null values 1024 1.00 2.1±0.00µs ? ?/sec 1.00 2.1±0.00µs ? ?/sec
take stringview null values null indices 1024 1.00 2.3±0.01µs ? ?/sec 1.06 2.4±0.04µs ? ?/sec
```
</p>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4042007055
🤖 `./gh_compare_arrow.sh` [gh_compare_arrow.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing improve-take-fsb (7aee3d92f3902078adebf4484a3a7421e5a8277b) to 33aed330b962d40e6e6b456bc4cd13ec80967f75 [diff](https://github.com/apache/arrow-rs/compare/33aed330b962d40e6e6b456bc4cd13ec80967f75..7aee3d92f3902078adebf4484a3a7421e5a8277b)
BENCH_NAME=take_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=improve-take-fsb
Results will be posted here when complete
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4038789105
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2920840966
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2945124235
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
+ }
+ } else {
+ // The indices nullability cannot be ignored here because the values buffer may contain
+ // nulls which should not cause a panic.
+ let array_iter = indices.iter().map(|idx| {
+ idx.map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ })
+ });
+ for slice in array_iter {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4041368616
run benchmark take_kernels
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2939123229
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4042006984
Will run once more to ensure we can reproduce the results. They look good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#issuecomment-4042006055
run benchmark take_kernels
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Optimize `take_fixed_size_binary` For Predefined Value Lengths [arrow-rs]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #9535:
URL: https://github.com/apache/arrow-rs/pull/9535#discussion_r2944670897
##########
arrow-select/src/take.rs:
##########
@@ -723,46 +724,128 @@ fn take_fixed_size_binary<IndexType: ArrowPrimitiveType>(
ArrowError::InvalidArgumentError(format!("Cannot convert size '{}' to usize", size))
})?;
- let values_buffer = values.values().as_slice();
- let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
-
- if indices.null_count() == 0 {
- let array_iter = indices.values().iter().map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- });
- for slice in array_iter {
- values_buffer_builder.append_slice(slice);
- }
- } else {
- // The indices nullability cannot be ignored here because the values buffer may contain
- // nulls which should not cause a panic.
- let array_iter = indices.iter().map(|idx| {
- idx.map(|idx| {
- let offset = idx.as_usize() * size_usize;
- &values_buffer[offset..offset + size_usize]
- })
- });
- for slice in array_iter {
- match slice {
- None => values_buffer_builder.append_n(size_usize, 0),
- Some(slice) => values_buffer_builder.append_slice(slice),
- }
- }
- }
+ let result_buffer = match size_usize {
+ 1 => take_fixed_size::<IndexType, 1>(values.values(), indices),
+ 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),
+ 16 => take_fixed_size::<IndexType, 16>(values.values(), indices),
+ _ => take_fixed_size_binary_buffer_dynamic_length(values, indices, size_usize),
+ };
- let values_buffer = values_buffer_builder.finish();
let value_nulls = take_nulls(values.nulls(), indices);
let final_nulls = NullBuffer::union(value_nulls.as_ref(), indices.nulls());
-
let array_data = ArrayDataBuilder::new(DataType::FixedSizeBinary(size))
.len(indices.len())
.nulls(final_nulls)
.offset(0)
- .add_buffer(values_buffer)
+ .add_buffer(result_buffer)
.build()?;
- Ok(FixedSizeBinaryArray::from(array_data))
+ return Ok(FixedSizeBinaryArray::from(array_data));
+
+ /// Implementation of the take kernel for fixed size binary arrays.
+ #[inline(never)]
+ fn take_fixed_size_binary_buffer_dynamic_length<IndexType: ArrowPrimitiveType>(
+ values: &FixedSizeBinaryArray,
+ indices: &PrimitiveArray<IndexType>,
+ size_usize: usize,
+ ) -> Buffer {
+ let values_buffer = values.values().as_slice();
+ let mut values_buffer_builder = BufferBuilder::new(indices.len() * size_usize);
+
+ if indices.null_count() == 0 {
+ let array_iter = indices.values().iter().map(|idx| {
+ let offset = idx.as_usize() * size_usize;
+ &values_buffer[offset..offset + size_usize]
+ });
+ for slice in array_iter {
+ values_buffer_builder.append_slice(slice);
Review Comment:
We could probably make this faster by copying constant-sized arrays instead of slices, also the builder APIs can have some non-trivial overhead sometimes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org