Skip to content

StructArray::try_new validation incorrectly returns an error when logical_nulls() returns Some() && null_count == 0 #7435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
phillipleblanc opened this issue Apr 23, 2025 · 0 comments · Fixed by #7436
Labels

Comments

@phillipleblanc
Copy link
Contributor

Describe the bug
This logic in StructArray::try_new validates that if one of the child arrays in the StructArray has a null value that isn't properly masked by the parent, then it will be rejected with the error Found unmasked nulls for non-nullable StructArray field.

if !f.is_nullable() {
    if let Some(a) = a.logical_nulls() {
        !nulls.as_ref().map(|n| n.contains(&a)).unwrap_or_default() {
            return Err(ArrowError::InvalidArgumentError(format!(
                "Found unmasked nulls for non-nullable StructArray field {:?}",
                f.name()
            )));
        }
    }
}

However, it is possible for a.logical_nulls() to return Some(_) and the NullBuffer itself to report that there aren't any nulls (null_count == 0), which leads StructArray::try_new to incorrectly report that it found unmasked nulls.

To Reproduce

#[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"));
}

Expected behavior
The validation succeeds.

Additional context

I will raise a PR for this fix shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
1 participant