You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Eyad3skr (via GitHub)" <gi...@apache.org> on 2026/02/13 18:38:16 UTC
[PR] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Eyad3skr opened a new pull request, #9411:
URL: https://github.com/apache/arrow-rs/pull/9411
Implements a helper to replace the pattern of creating a `BooleanBuffer` from an unsliced validity bitmap and filtering by null count. Previously this was done with `BooleanBuffer::new(...)` plus `Some(NullBuffer::new(...)).filter(|n| n.null_count() > 0);` now it is a single call to` NullBuffer::try_from_unsliced(buffer, len)`, which returns `Some(NullBuffer)` when there are nulls and `None` when all values are valid.
- Added `try_from_unsliced` in `arrow-buffer/src/buffer/null.rs` with tests for nulls, all valid, all null, empty
- Refactor `FixedSizeBinaryArray::try_from_iter_with_size` and `try_from_sparse_iter_with_size` to use it
- Refactor `take_nulls` in `arrow-select` to use it
Closes #9385
--
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] Add `NullBuffer::from_unsliced_buffer` helper and refactor call sites [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2806284603
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -222,6 +222,14 @@ impl NullBuffer {
pub fn buffer(&self) -> &Buffer {
self.buffer.inner()
}
+ /// Create a [`NullBuffer`] from an *unsliced* validity bitmap (`offset = 0`) of length `len`.
Review Comment:
Could you also update this comment to reflect the offset is in **bits** (not bytes)? Mixing the units is a common mistake so making sure the documentation is as clear as possible would help
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3905045589
@alamb I guess there is nothing else to take care after anymore? maybe if someone can just trigger/approve the CI pipeline workflows left that would be awesome.
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2834899827
##########
arrow-string/src/substring.rs:
##########
@@ -356,15 +355,16 @@ fn fixed_size_binary_substring(
let mut nulls = array
.nulls()
.map(|n| n.inner().sliced())
- .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements)))
- .filter(|n| n.null_count() > 0);
+ .and_then(|b| NullBuffer::from_unsliced_buffer(b, num_of_elements));
+
+ // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
+ // In that case it uses the null buffer length, so preserve the original length here.
+ // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
+ // otherwise it collapses to an empty array (len=0).
if new_len == 0 && nulls.is_none() {
- // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
- // In that case it uses the null buffer length, so preserve the original length here.
- // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
- // otherwise it collapses to an empty array (len=0).
Review Comment:
fair enough
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3937013437
> LGTM! But I guess CI will fail due to formatting/import issues.
>
> You could run `cargo fmt` and `cargo check` to check it out
true, I forgot about 2 warning from unused imports.. thanks!
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3907941320
> @alamb I guess there is nothing else to take care after anymore? maybe if someone can just trigger/approve the CI pipeline workflows left that would be awesome.
Done!
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2834859760
##########
arrow-string/src/substring.rs:
##########
@@ -356,15 +355,13 @@ fn fixed_size_binary_substring(
let mut nulls = array
.nulls()
.map(|n| n.inner().sliced())
- .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements)))
- .filter(|n| n.null_count() > 0);
+ .and_then(|b| NullBuffer::from_unsliced_buffer(b, num_of_elements));
+
+ // FixedSizeBinaryArray with size 0 requires a validity bitmap
if new_len == 0 && nulls.is_none() {
- // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
- // In that case it uses the null buffer length, so preserve the original length here.
- // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
- // otherwise it collapses to an empty array (len=0).
Review Comment:
On it!
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2807230204
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -222,6 +222,14 @@ impl NullBuffer {
pub fn buffer(&self) -> &Buffer {
self.buffer.inner()
}
+ /// Create a [`NullBuffer`] from an *unsliced* validity bitmap (`offset = 0`) of length `len`.
+ ///
+ /// Returns `None` if there are no nulls (all values valid).
+ pub fn try_from_unsliced(buffer: impl Into<Buffer>, len: usize) -> Option<Self> {
Review Comment:
a valid point tbh! on it.
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3914602489
Seems like the CI is broken
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2834898347
##########
arrow-string/src/substring.rs:
##########
@@ -356,15 +355,16 @@ fn fixed_size_binary_substring(
let mut nulls = array
.nulls()
.map(|n| n.inner().sliced())
- .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements)))
- .filter(|n| n.null_count() > 0);
+ .and_then(|b| NullBuffer::from_unsliced_buffer(b, num_of_elements));
+
+ // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
+ // In that case it uses the null buffer length, so preserve the original length here.
+ // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
+ // otherwise it collapses to an empty array (len=0).
if new_len == 0 && nulls.is_none() {
- // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
- // In that case it uses the null buffer length, so preserve the original length here.
- // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
- // otherwise it collapses to an empty array (len=0).
Review Comment:
nit: can we move the comment to its original place?
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2825146176
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -274,4 +284,49 @@ mod tests {
std::mem::size_of::<Option<NullBuffer>>()
);
}
+
+ #[test]
+ fn test_from_unsliced_buffer_with_nulls() {
+ // Buffer with some nulls: 0b10110010 = valid, null, valid, valid, null, null, valid, null
+ let buf = Buffer::from([0b10110010u8]);
Review Comment:
Arrow uses LSB numbering and that's why this test failed. Could refer to the [doc](https://arrow.apache.org/docs/format/Columnar.html#validity-bitmaps) to fix the test
--
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] Add `NullBuffer::from_unsliced_buffer` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3939679180
would appreciate if you can trigger the CI workflows for me @alamb
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3921929827
true, my schedule this week is a bit busy because of university. I will be working on fixing the CI between today and Friday. On it.
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2833238320
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -274,4 +284,49 @@ mod tests {
std::mem::size_of::<Option<NullBuffer>>()
);
}
+
+ #[test]
+ fn test_from_unsliced_buffer_with_nulls() {
+ // Buffer with some nulls: 0b10110010 = valid, null, valid, valid, null, null, valid, null
+ let buf = Buffer::from([0b10110010u8]);
Review Comment:
Oh!! Thanks for the info. going to check it now!
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "liamzwbao (via GitHub)" <gi...@apache.org>.
liamzwbao commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2834828969
##########
arrow-string/src/regexp.rs:
##########
@@ -180,6 +180,11 @@ pub fn regexp_is_match_scalar<'a, S>(
where
&'a S: StringArrayType<'a>,
{
+ let null_bit_buffer = array
+ .nulls()
+ .map(|n| n.inner().sliced())
+ .and_then(|b| NullBuffer::from_unsliced_buffer(b, array.len()))
+ .map(|nb| nb.into_inner().into_inner());
Review Comment:
We don't need a new `null_bit_buffer`, let's refactor line 208 to 212 at the end of this func instead
##########
arrow-string/src/substring.rs:
##########
@@ -356,15 +355,13 @@ fn fixed_size_binary_substring(
let mut nulls = array
.nulls()
.map(|n| n.inner().sliced())
- .map(|b| NullBuffer::new(BooleanBuffer::new(b, 0, num_of_elements)))
- .filter(|n| n.null_count() > 0);
+ .and_then(|b| NullBuffer::from_unsliced_buffer(b, num_of_elements));
+
+ // FixedSizeBinaryArray with size 0 requires a validity bitmap
if new_len == 0 && nulls.is_none() {
- // FixedSizeBinaryArray::new takes length from the values buffer, except when size == 0.
- // In that case it uses the null buffer length, so preserve the original length here.
- // Example: ["", "", ""] -> substring(..., 1, Some(2)) should keep len=3;
- // otherwise it collapses to an empty array (len=0).
Review Comment:
We should keep the existing comment here and avoid adding the new comment on line 360, as it is unrelated to this PR.
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Eyad3skr (via GitHub)" <gi...@apache.org>.
Eyad3skr commented on PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#issuecomment-3936783252
would appreciate if you can trigger/approve on CI workflows @alamb
--
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] Add `NullBuffer::try_from_unsliced` helper and refactor call sites [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #9411:
URL: https://github.com/apache/arrow-rs/pull/9411#discussion_r2806876256
##########
arrow-buffer/src/buffer/null.rs:
##########
@@ -222,6 +222,14 @@ impl NullBuffer {
pub fn buffer(&self) -> &Buffer {
self.buffer.inner()
}
+ /// Create a [`NullBuffer`] from an *unsliced* validity bitmap (`offset = 0`) of length `len`.
+ ///
+ /// Returns `None` if there are no nulls (all values valid).
+ pub fn try_from_unsliced(buffer: impl Into<Buffer>, len: usize) -> Option<Self> {
Review Comment:
Maybe we should call it `from_buffer`? `try_*` probably should be reserved for functions that return a `result`, and its probably better to be direct that we're constructing directly from a `buffer`?
--
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