You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "phillipleblanc (via GitHub)" <gi...@apache.org> on 2025/04/23 15:13:48 UTC
[PR] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
phillipleblanc opened a new pull request, #7436:
URL: https://github.com/apache/arrow-rs/pull/7436
# Which issue does this PR close?
- Closes #7435
# Rationale for this change
Fixes the validation logic in StructArray::try_new to not error if there aren't any unmasked nulls, but `a.logical_nulls()` returns Some
# What changes are included in this PR?
A single line change to check for `null_count()` on the `NullBuffer` before comparing against the parent nulls.
# 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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#discussion_r2080718676
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -146,7 +146,9 @@ impl StructArray {
if !f.is_nullable() {
if let Some(a) = a.logical_nulls() {
- if !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default() {
+ if a.null_count() > 0
+ && !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default()
Review Comment:
I've fixed this
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -842,4 +844,23 @@ mod tests {
);
assert_eq!(format!("{arr:?}"), "StructArray\n-- validity:\n[\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n ...10 elements...,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n]\n[\n-- child 0: \"c\" (Int32)\nPrimitiveArray<Int32>\n[\n 0,\n 1,\n 2,\n 3,\n 4,\n 5,\n 6,\n 7,\n 8,\n 9,\n ...10 elements...,\n 20,\n 21,\n 22,\n 23,\n 24,\n 25,\n 26,\n 27,\n 28,\n 29,\n]\n]")
}
+
+ #[test]
+ fn test_struct_array_logical_nulls() {
+ // Field is non-nullable
+ let field = Field::new("a", DataType::Int32, false);
+ let values = vec![1, 2, 3];
+ // Create a NullBuffer with all bits set to valid (true)
+ let nulls = NullBuffer::from(vec![true, true, true]);
+ let array = Int32Array::new(values.into(), Some(nulls));
+ let child = Arc::new(array) as ArrayRef;
+ assert!(child.logical_nulls().is_some());
+ assert_eq!(child.logical_nulls().unwrap().null_count(), 0);
+
+ let fields = Fields::from(vec![field]);
+ let arrays = vec![child];
+ let nulls = None;
+
+ drop(StructArray::try_new(fields, arrays, nulls).expect("should not error"));
Review Comment:
Fixed
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#issuecomment-2864841946
Hmm, I'm not sure why the integration test failed - can we try re-running?
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#issuecomment-2864819043
Thanks for taking a look @alamb - this is ready for another review.
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#discussion_r2080213795
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -842,4 +844,23 @@ mod tests {
);
assert_eq!(format!("{arr:?}"), "StructArray\n-- validity:\n[\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n ...10 elements...,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n]\n[\n-- child 0: \"c\" (Int32)\nPrimitiveArray<Int32>\n[\n 0,\n 1,\n 2,\n 3,\n 4,\n 5,\n 6,\n 7,\n 8,\n 9,\n ...10 elements...,\n 20,\n 21,\n 22,\n 23,\n 24,\n 25,\n 26,\n 27,\n 28,\n 29,\n]\n]")
}
+
+ #[test]
+ fn test_struct_array_logical_nulls() {
+ // Field is non-nullable
+ let field = Field::new("a", DataType::Int32, false);
+ let values = vec![1, 2, 3];
+ // Create a NullBuffer with all bits set to valid (true)
+ let nulls = NullBuffer::from(vec![true, true, true]);
+ let array = Int32Array::new(values.into(), Some(nulls));
+ let child = Arc::new(array) as ArrayRef;
+ assert!(child.logical_nulls().is_some());
+ assert_eq!(child.logical_nulls().unwrap().null_count(), 0);
+
+ let fields = Fields::from(vec![field]);
+ let arrays = vec![child];
+ let nulls = None;
+
+ drop(StructArray::try_new(fields, arrays, nulls).expect("should not error"));
Review Comment:
Is there a reason to have `drop` here?
This seemed to work for me too
```suggestion
StructArray::try_new(fields, arrays, nulls).expect("should not error");
```
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -146,7 +146,9 @@ impl StructArray {
if !f.is_nullable() {
if let Some(a) = a.logical_nulls() {
- if !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default() {
+ if a.null_count() > 0
+ && !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default()
Review Comment:
It probably isn't measurable, but as written this will now always count nulls even if they are unmasked
I think it would be better if we only count the nulls right before we error to avoid unecessary work
```suggestion
if !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default()
&& a.null_count() > 0
```
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#discussion_r2080689877
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -842,4 +844,23 @@ mod tests {
);
assert_eq!(format!("{arr:?}"), "StructArray\n-- validity:\n[\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n ...10 elements...,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n valid,\n null,\n]\n[\n-- child 0: \"c\" (Int32)\nPrimitiveArray<Int32>\n[\n 0,\n 1,\n 2,\n 3,\n 4,\n 5,\n 6,\n 7,\n 8,\n 9,\n ...10 elements...,\n 20,\n 21,\n 22,\n 23,\n 24,\n 25,\n 26,\n 27,\n 28,\n 29,\n]\n]")
}
+
+ #[test]
+ fn test_struct_array_logical_nulls() {
+ // Field is non-nullable
+ let field = Field::new("a", DataType::Int32, false);
+ let values = vec![1, 2, 3];
+ // Create a NullBuffer with all bits set to valid (true)
+ let nulls = NullBuffer::from(vec![true, true, true]);
+ let array = Int32Array::new(values.into(), Some(nulls));
+ let child = Arc::new(array) as ArrayRef;
+ assert!(child.logical_nulls().is_some());
+ assert_eq!(child.logical_nulls().unwrap().null_count(), 0);
+
+ let fields = Fields::from(vec![field]);
+ let arrays = vec![child];
+ let nulls = None;
+
+ drop(StructArray::try_new(fields, arrays, nulls).expect("should not error"));
Review Comment:
It was my linter complaining about an unused value, `let _ = ` was the alternative. But I may have stricter lint setting than this repo, so I'll change 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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#discussion_r2080690194
##########
arrow-array/src/array/struct_array.rs:
##########
@@ -146,7 +146,9 @@ impl StructArray {
if !f.is_nullable() {
if let Some(a) = a.logical_nulls() {
- if !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default() {
+ if a.null_count() > 0
+ && !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default()
Review Comment:
Yes, I will change this. Thanks for catching.
--
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] Fix validation logic in `StructArray::try_new` to account for array.logical_nulls() returning Some() and null_count == 0 [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7436:
URL: https://github.com/apache/arrow-rs/pull/7436#issuecomment-2867298507
Thanks @phillipleblanc
--
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