Rich-T-kid commented on code in PR #10994:
URL: https://github.com/apache/arrow-rs/pull/10994#discussion_r3937029603
##########
arrow-select/src/take.rs:
##########
@@ -339,6 +339,12 @@ fn take_impl<IndexType: ArrowPrimitiveType, const CHECKED:
bool>(
}
}
DataType::Union(fields, UnionMode::Sparse) => {
+ if indices.null_count() > 0 && fields.iter().any(|(_, field)|
!field.is_nullable()) {
Review Comment:
this seems to introduce a bug
```rust
#[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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
