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 2025/11/21 13:42:54 UTC
[PR] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
tobixdev opened a new pull request, #8901:
URL: https://github.com/apache/arrow-rs/pull/8901
# Which issue does this PR close?
- Closes #8900 .
# Rationale for this change
This causes the values buffer to have the expected length after creating the null array.
# What changes are included in this PR?
Use `MutableBuffer::new_null` instead of `MutableBuffer::new`
# Are these changes tested?
Yes, additional constructor test
# Are there any user-facing changes?
Yes, the buffer will now be correctly initialized when calling `FixedSizeBinaryArray::new_null`
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#issuecomment-3573074873
> I think zeroing is not required for null values based on the arrow specification. But I guess not doing it has some foot guns and UB issues that we want to avoid. Curious if there is a take on that from arrow-rs. 🤔
Allocating anything in rust is going to zero out the bytes anyways. Unless there is a compelling performance benchmark that shows avoiding the (potentially) extra zeroing makes a non trivial difference I think we should opt for safe
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#discussion_r2550603957
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -119,10 +119,10 @@ impl FixedSizeBinaryArray {
/// * `size < 0`
/// * `size * len` would overflow `usize`
pub fn new_null(size: i32, len: usize) -> Self {
- let capacity = size.to_usize().unwrap().checked_mul(len).unwrap();
+ let capacity_in_bytes = size.to_usize().unwrap().checked_mul(len).unwrap();
Self {
data_type: DataType::FixedSizeBinary(size),
- value_data: MutableBuffer::new(capacity).into(),
+ value_data: MutableBuffer::new_null(capacity_in_bytes * 8).into(),
Review Comment:
```suggestion
// MutableBuffer::new_null is in bits.
value_data: MutableBuffer::new_null(capacity_in_bytes * 8).into(),
```
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#issuecomment-3566682001
🚀
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on code in PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#discussion_r2549808299
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -983,6 +983,10 @@ mod tests {
let nulls = NullBuffer::new_null(5);
FixedSizeBinaryArray::new(2, buffer.clone(), Some(nulls));
+ let null_array = FixedSizeBinaryArray::new_null(4, 3);
+ assert_eq!(null_array.len(), 3);
+ assert_eq!(null_array.values().len(), 12);
Review Comment:
This reported 0 before this change.
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#discussion_r2550245086
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -119,10 +119,10 @@ impl FixedSizeBinaryArray {
/// * `size < 0`
/// * `size * len` would overflow `usize`
pub fn new_null(size: i32, len: usize) -> Self {
- let capacity = size.to_usize().unwrap().checked_mul(len).unwrap();
+ let capacity_in_bytes = size.to_usize().unwrap().checked_mul(len).unwrap();
Self {
data_type: DataType::FixedSizeBinary(size),
- value_data: MutableBuffer::new(capacity).into(),
+ value_data: MutableBuffer::new_null(capacity_in_bytes * 8).into(),
Review Comment:
Where does the `8` come from?
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#issuecomment-3564231609
I also think we need to prioritize correctness over performance (obviously 😆 ) -- there isn't any obvious way to make this faster that I could see 🤔
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "tobixdev (via GitHub)" <gi...@apache.org>.
tobixdev commented on PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#issuecomment-3569527840
> The only downside I can think of is the extra memset, but that would be done for primitive arrays as well IIUC.
I think zeroing is not required for null values based on the arrow specification. But I guess not doing it has some foot guns and UB issues that we want to avoid. Curious if there is a take on that from arrow-rs. :thinking:
Thanks for merging that! I'll make a follow-up PR that adds the small comment.
--
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] Allocate a zeroed buffer for FixedSizeBinaryArray::null [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8901:
URL: https://github.com/apache/arrow-rs/pull/8901#discussion_r2550516706
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -119,10 +119,10 @@ impl FixedSizeBinaryArray {
/// * `size < 0`
/// * `size * len` would overflow `usize`
pub fn new_null(size: i32, len: usize) -> Self {
- let capacity = size.to_usize().unwrap().checked_mul(len).unwrap();
+ let capacity_in_bytes = size.to_usize().unwrap().checked_mul(len).unwrap();
Self {
data_type: DataType::FixedSizeBinary(size),
- value_data: MutableBuffer::new(capacity).into(),
+ value_data: MutableBuffer::new_null(capacity_in_bytes * 8).into(),
Review Comment:
`MutableBuffer::new_null` is in bits.
--
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