This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 9928b7c6b7 fix: `try_binary` dropped the logical nulls of run and
dictionary arrays (#10748)
9928b7c6b7 is described below
commit 9928b7c6b7636ea2cbbedb212faa927de8fcccdd
Author: Emil Ernerfeldt <[email protected]>
AuthorDate: Fri Aug 21 21:34:45 2026 -0700
fix: `try_binary` dropped the logical nulls of run and dictionary arrays
(#10748)
# Which issue does this PR close?
No issue; found while reviewing
https://github.com/apache/arrow-rs/pull/10730.
# Rationale for this change
`try_binary` skipped null handling when `a.null_count() == 0 &&
b.null_count() == 0`. That is a *physical* count. A `RunArray` never has
a null buffer of its own, so an array whose values contain nulls took
the no-nulls fast path and silently produced a result with no nulls at
all:
```
values [Some(10), None, Some(30)] + [1, 1, 1] => [11, 1, 31] // row 1
should be null
```
A `DictionaryArray` with nullable values has the same problem. `binary`
is unaffected: it has no such fast path.
# What changes are included in this PR?
Gate the fast path on `is_nullable`, which accounts for logical nulls.
That check is allowed to be conservative, so an empty union of the
logical nulls now falls back to the no-nulls path instead of
`unwrap`ping.
# Are these changes tested?
Yes, a new test for a `RunArray` with nulls in its values. It fails on
`main`.
# Are there any user-facing changes?
`try_binary` returns the correct nulls for run and dictionary arrays. No
API change.
---------
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
arrow-arith/src/arity.rs | 51 ++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/arrow-arith/src/arity.rs b/arrow-arith/src/arity.rs
index f15ea955ea..42aa5b0889 100644
--- a/arrow-arith/src/arity.rs
+++ b/arrow-arith/src/arity.rs
@@ -270,11 +270,17 @@ where
}
let len = a.len();
- if a.null_count() == 0 && b.null_count() == 0 {
+ // Physical nulls are not the whole story: a `RunArray` or
`DictionaryArray` can have
+ // logical nulls in its values while its own null buffer is absent.
`is_nullable` covers
+ // those, but is allowed to be conservative, so the union of the logical
nulls can still
+ // be empty.
+ if !a.is_nullable() && !b.is_nullable() {
try_binary_no_nulls(len, a, b, op)
} else {
- let nulls =
- NullBuffer::union(a.logical_nulls().as_ref(),
b.logical_nulls().as_ref()).unwrap();
+ let Some(nulls) = NullBuffer::union(a.logical_nulls().as_ref(),
b.logical_nulls().as_ref())
+ else {
+ return try_binary_no_nulls(len, a, b, op);
+ };
let mut buffer = BufferBuilder::<O::Native>::new(len);
buffer.append_n_zeroed(len);
@@ -324,12 +330,17 @@ where
))));
}
- if a.null_count() == 0 && b.null_count() == 0 {
+ // Physical and logical nulls coincide for a `PrimitiveArray`, but gate on
`is_nullable` to
+ // match `try_binary`. That check is allowed to be conservative, so fall
back to the no-nulls
+ // path when the union of the logical nulls turns out to be empty, instead
of unwrapping it.
+ if !a.is_nullable() && !b.is_nullable() {
try_binary_no_nulls_mut(len, a, b, op)
} else {
- let nulls =
+ let Some(nulls) =
create_union_null_buffer(a.logical_nulls().as_ref(),
b.logical_nulls().as_ref())
- .unwrap();
+ else {
+ return try_binary_no_nulls_mut(len, a, b, op);
+ };
let mut builder = a.into_builder()?;
@@ -431,6 +442,22 @@ mod tests {
);
}
+ #[test]
+ fn test_try_binary_run_array_logical_nulls() {
+ // A `RunArray` has no null buffer of its own, so its logical nulls
live in the values.
+ let run_ends = Int32Array::from(vec![1, 2, 3]);
+ let values = Int32Array::from(vec![Some(10), None, Some(30)]);
+ let run = RunArray::<Int32Type>::try_new(&run_ends,
&values).expect("valid run array");
+ assert_eq!(run.null_count(), 0);
+ assert_eq!(run.logical_null_count(), 1);
+
+ let typed = run.downcast::<Int32Array>().expect("Int32 values");
+ let other = Int32Array::from(vec![1, 1, 1]);
+ let result =
+ try_binary::<_, _, _, Int32Type>(typed, &other, |a, b| Ok(a +
b)).expect("no overflow");
+ assert_eq!(result, Int32Array::from(vec![Some(11), None, Some(31)]));
+ }
+
#[test]
fn test_binary_mut() {
let a = Int32Array::from(vec![15, 14, 9, 8, 1]);
@@ -509,6 +536,18 @@ mod tests {
assert_eq!(r1.unwrap(), r2.unwrap());
}
+ #[test]
+ fn test_try_binary_mut_all_valid_null_buffers() {
+ // Both arrays carry a null buffer with no nulls in it: the no-nulls
path must still run.
+ let a = Int32Array::new(vec![1, 2].into(), Some(vec![true,
true].into()));
+ let b = Int32Array::new(vec![10, 20].into(), Some(vec![true,
true].into()));
+ let c = try_binary_mut(a, &b, |a, b| Ok(a + b))
+ .expect("not shared")
+ .expect("no overflow");
+ assert_eq!(c, Int32Array::from(vec![11, 22]));
+ assert_eq!(c.logical_null_count(), 0);
+ }
+
#[test]
fn test_unary_dict_mut() {
let values = Int32Array::from(vec![Some(10), Some(20), None]);
