fix(arrow-select): preserve nullability for REE and Union take - #10994
fix(arrow-select): preserve nullability for REE and Union take#10994yongster wants to merge 2 commits into
Conversation
|
I also reviewed I did not identify the same metadata-consistency issue in these kernels: they |
Rich-T-kid
left a comment
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()) { |
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?
There was a problem hiding this comment.
i think this is an existing issue with union builders?
There was a problem hiding this comment.
ah, your right. interesting bug
| } | ||
| } | ||
| DataType::Union(fields, UnionMode::Sparse) => { | ||
| if indices.null_count() > 0 && fields.iter().any(|(_, field)| !field.is_nullable()) { |
There was a problem hiding this comment.
technically this might be too strict? as long as we have one child that is nullable we can select that type id, then for the rest we fill in nulls or some default; it shouldnt matter since they arent the selected child, even if their values arent null
|
from a codex review it identified 2 edge cases:
(these are pretty extreme edge cases honestly, i wouldnt mind them being a separate issue; just wanted to point out for completeness) |
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.