fix(arrow-select): preserve nullability for REE and Union take - #10994
Open
yongster wants to merge 1 commit into
Open
fix(arrow-select): preserve nullability for REE and Union take#10994yongster wants to merge 1 commit into
yongster wants to merge 1 commit into
Conversation
Contributor
Author
|
I also reviewed I did not identify the same metadata-consistency issue in these kernels: they |
Rich-T-kid
reviewed
Sep 4, 2026
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
Looking at the arrow-spec, this looks mostly correct.
| } | ||
| } | ||
| DataType::Union(fields, UnionMode::Sparse) => { | ||
| if indices.null_count() > 0 && fields.iter().any(|(_, field)| !field.is_nullable()) { |
Contributor
There was a problem hiding this comment.
this seems to introduce a bug
#[test]
fn test_take_union_builder_null_index_regression() {
let mut builder = UnionBuilder::new_dense();
builder.append::<Int32Type>("a", 10).unwrap();
builder.append_null::<Int32Type>("a").unwrap();
let union = builder.build().unwrap();
// UnionBuilder currently declares union fields as non-nullable.
let field = union.fields().iter().next().unwrap().1;
assert!(!field.is_nullable());
// But it still represents logical nulls through the selected child.
assert!(union.logical_nulls().unwrap().is_null(1));
let indices = UInt32Array::from(vec![Some(0), None, Some(1)]);
// This should preserve take's normal null-index contract:
// a null index produces a logical null in the output.
let taken = take(&union, &indices, None).unwrap();
let taken = taken.as_union();
let logical_nulls = taken.logical_nulls().unwrap();
assert!(logical_nulls.is_valid(0));
assert!(logical_nulls.is_null(1));
assert!(logical_nulls.is_null(2));
}with this PR at the take() call would cause an error. is this intended?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
takecan introduce output nulls when its indices contain nulls.Most Arrow arrays represent these with a top-level validity bitmap. However,
RunEndEncoded and Union arrays derive logical nullability from their child
arrays:
valuesfield.Previously,
takecould write nulls into children whose corresponding fieldmetadata was marked as non-nullable. This made the physical output inconsistent
with its declared schema.
Closes Define nullability semantics for kernels on REE and Union arrays #10992.
Return a compute error when null take indices would introduce nulls into a
RunEndEncoded array with a non-nullable
valuesfield.For Dense Union arrays, select a nullable child to represent null take
indices.
Return a compute error for Sparse Union arrays with non-nullable fields when
take indices contain nulls, since every child would otherwise receive an
introduced null.
Add regression tests for each behavior.
This complements #10909 and ensures that arrays marked as non-nullable do not
produce output containing newly introduced nulls.