Skip to content

Commit cee4114

Browse files
cool9850311claude
cool9850311
and
claude
authored
fix: return NULL from bit_xor when a sliding frame has no non-NULL values (#26485)
## Which issue does this PR close? - Closes #26470. ## Rationale for this change `bit_xor` over a sliding window frame returns `0` instead of `NULL` once every non-NULL value has left the frame: ```sql SELECT x, bit_xor(x) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND CURRENT ROW) AS sliding, bit_xor(x) OVER (PARTITION BY i) AS whole FROM (VALUES (1, 5), (2, NULL), (3, 6)) t(i, x) ORDER BY i; +------+---------+-------+ | x | sliding | whole | +------+---------+-------+ | 5 | 5 | 5 | | NULL | 0 | NULL | <-- sliding should be NULL, as `whole` is | 6 | 6 | 6 | +------+---------+-------+ ``` An aggregate with no non-NULL input returns NULL, and `bit_xor` is documented as computing "the bitwise exclusive OR of all non-null input values". The `0` is the XOR identity leaking out of the accumulator's internal state. DuckDB returns NULL here. The cause is that `BitXorAccumulator` keeps a single `Option<T::Native>` whose `None` doubles as "has not seen a non-NULL value yet". `retract_batch` can undo the value, since XOR is its own inverse, but it can never move the state back from `Some` to `None`. So after the last non-NULL value leaves the frame the accumulator is left holding `Some(0)`. Over a whole partition the same aggregate is correct, because that path never retracts. ## What changes are included in this PR? `BitXorAccumulator` now tracks the number of non-NULL values currently accumulated and returns NULL when it is zero, the same way `avg` does (#22139). `retract_batch` no longer delegates to `update_batch`, since the count has to move the other way. A genuine zero is preserved: `bit_xor` of `[5, 5]` still returns `0`. `state_fields` is deliberately unchanged, so the state schema and its serialization are untouched. The count never has to be serialized: `state()` already yields NULL when there is no non-NULL input, and `merge_batch` goes through `update_batch`, which adds nothing for a NULL state and one for a non-NULL state. `bit_and`, `bit_or` and `bit_xor(DISTINCT ..)` do not implement `retract_batch`, so they cannot reach this state machine and are unaffected. ## What is the testing strategy for this PR? Two layers: - Four `sqllogictest` cases in `datafusion/sqllogictest/test_files/window.slt`, placed next to the analogous `avg` regression tests: the issue's own reproduction (the sliding frame compared against the whole partition), a narrow frame that drains to empty on every row, all-NULL input, and a case pinning down that a legitimate `0` survives and that NULL follows it. - Two accumulator unit tests in `datafusion/functions-aggregate/src/bit_and_or_xor.rs`: the retract-to-empty state machine, including that retracting NULLs does not move the count and that the accumulator recovers when a non-NULL value re-enters, and the legitimate-zero case. I checked the tests fail without the fix: reverting the change to the accumulator and keeping only the new tests turns three of the four slt blocks red, each reporting `0` where NULL is expected. The all-NULL block passes either way, so it is a guard rather than a reproduction. I also verified the partial/final merge path directly, since `GROUP BY` uses `PrimitiveGroupsAccumulator` and does not exercise this accumulator's `state()`/`merge_batch()`: a partial over `[5, 5]` emits `0` as its state and the final phase returns `0`, while merging only NULL states returns NULL. The full test suite passes (`cargo test --profile ci --workspace --lib --tests --bins` with the extended feature set), as do `cargo fmt --all` and `ci/scripts/rust_clippy.sh`. ## Are there any user-facing changes? Yes, a bug fix: `bit_xor` over a sliding window frame that contains no non-NULL values now returns NULL instead of `0`. No public API changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <[email protected]>
1 parent 2c2ecae commit cee4114

2 files changed

Lines changed: 118 additions & 8 deletions

File tree

‎datafusion/functions-aggregate/src/bit_and_or_xor.rs‎

Lines changed: 69 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,11 @@ where
433433
}
434434

435435
struct BitXorAccumulator<T: ArrowNumericType> {
436-
value: Option<T::Native>,
436+
/// Running XOR of every value seen so far, starting from the identity `0`.
437+
/// Only meaningful when `non_null_count` is non-zero.
438+
value: T::Native,
439+
/// Number of non-NULL values currently accumulated (inserted minus retracted)
440+
non_null_count: usize,
437441
}
438442

439443
impl<T: ArrowNumericType> std::fmt::Debug for BitXorAccumulator<T> {
@@ -444,7 +448,10 @@ impl<T: ArrowNumericType> std::fmt::Debug for BitXorAccumulator<T> {
444448

445449
impl<T: ArrowNumericType> Default for BitXorAccumulator<T> {
446450
fn default() -> Self {
447-
Self { value: None }
451+
Self {
452+
value: T::Native::usize_as(0),
453+
non_null_count: 0,
454+
}
448455
}
449456
}
450457

@@ -454,23 +461,30 @@ where
454461
{
455462
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
456463
if let Some(x) = arrow::compute::bit_xor(values[0].as_primitive::<T>()) {
457-
let v = self.value.get_or_insert_with(|| T::Native::usize_as(0));
458-
*v = *v ^ x;
464+
self.value = self.value ^ x;
459465
}
466+
self.non_null_count += values[0].len() - values[0].null_count();
460467
Ok(())
461468
}
462469

463470
fn retract_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
464-
// XOR is it's own inverse
465-
self.update_batch(values)
471+
// XOR is its own inverse, so the value is updated as in `update_batch`;
472+
// only the count of non-NULL values moves the other way
473+
if let Some(x) = arrow::compute::bit_xor(values[0].as_primitive::<T>()) {
474+
self.value = self.value ^ x;
475+
}
476+
self.non_null_count -= values[0].len() - values[0].null_count();
477+
Ok(())
466478
}
467479

468480
fn supports_retract_batch(&self) -> bool {
469481
true
470482
}
471483

472484
fn evaluate(&mut self) -> Result<ScalarValue> {
473-
ScalarValue::new_primitive::<T>(self.value, &T::DATA_TYPE)
485+
// All non-NULL values may have been retracted, in which case the result is NULL
486+
let value = (self.non_null_count > 0).then_some(self.value);
487+
ScalarValue::new_primitive::<T>(value, &T::DATA_TYPE)
474488
}
475489

476490
fn size(&self) -> usize {
@@ -580,7 +594,7 @@ mod tests {
580594

581595
#[test]
582596
fn test_bit_xor_accumulator() {
583-
let mut accumulator = BitXorAccumulator::<UInt64Type> { value: None };
597+
let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
584598
let batches: Vec<_> = vec![vec![1, 2], vec![1]]
585599
.into_iter()
586600
.map(|b| Arc::new(b.into_iter().collect::<UInt64Array>()) as ArrayRef)
@@ -603,4 +617,51 @@ mod tests {
603617
ScalarValue::UInt64(Some(2))
604618
);
605619
}
620+
621+
#[test]
622+
fn test_bit_xor_accumulator_retract_to_all_null() {
623+
let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
624+
let five = [Arc::new(UInt64Array::from(vec![Some(5)])) as ArrayRef];
625+
let nulls = [Arc::new(UInt64Array::from(vec![None])) as ArrayRef];
626+
let seven = [Arc::new(UInt64Array::from(vec![Some(7)])) as ArrayRef];
627+
628+
accumulator.update_batch(&five).unwrap();
629+
assert_eq!(
630+
accumulator.evaluate().unwrap(),
631+
ScalarValue::UInt64(Some(5))
632+
);
633+
634+
accumulator.update_batch(&nulls).unwrap();
635+
assert_eq!(
636+
accumulator.evaluate().unwrap(),
637+
ScalarValue::UInt64(Some(5))
638+
);
639+
640+
// All non-NULL values left the frame, so the result is NULL
641+
accumulator.retract_batch(&five).unwrap();
642+
assert_eq!(accumulator.evaluate().unwrap(), ScalarValue::UInt64(None));
643+
644+
// Retracting NULLs must not move the count
645+
accumulator.retract_batch(&nulls).unwrap();
646+
assert_eq!(accumulator.evaluate().unwrap(), ScalarValue::UInt64(None));
647+
648+
accumulator.update_batch(&seven).unwrap();
649+
assert_eq!(
650+
accumulator.evaluate().unwrap(),
651+
ScalarValue::UInt64(Some(7))
652+
);
653+
}
654+
655+
#[test]
656+
fn test_bit_xor_accumulator_legitimate_zero() {
657+
let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
658+
let values = [Arc::new(UInt64Array::from(vec![Some(5), Some(5)])) as ArrayRef];
659+
660+
// 5 ^ 5 is a real zero, not NULL
661+
accumulator.update_batch(&values).unwrap();
662+
assert_eq!(
663+
accumulator.evaluate().unwrap(),
664+
ScalarValue::UInt64(Some(0))
665+
);
666+
}
606667
}

‎datafusion/sqllogictest/test_files/window.slt‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6736,6 +6736,55 @@ ORDER BY i;
67366736
3 1
67376737
4 NULL
67386738

6739+
# bit_xor over a sliding window must yield NULL when the frame has no non-NULL
6740+
# values, matching the result over the whole partition (apache/datafusion#26470).
6741+
# Before the fix the sliding result was 0 on the NULL row.
6742+
query III
6743+
SELECT x,
6744+
bit_xor(x) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND CURRENT ROW) AS sliding,
6745+
bit_xor(x) OVER (PARTITION BY i) AS whole
6746+
FROM (VALUES (1, 5), (2, CAST(NULL AS INT)), (3, 6)) t(i, x)
6747+
ORDER BY i;
6748+
----
6749+
5 5 5
6750+
NULL NULL NULL
6751+
6 6 6
6752+
6753+
# Narrow sliding frame that drains to empty each row.
6754+
query II
6755+
SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING)
6756+
FROM (VALUES(1,CAST(NULL AS INT)),(2,1),(3,CAST(NULL AS INT)),(4,CAST(NULL AS INT))) t(i,v)
6757+
ORDER BY i;
6758+
----
6759+
1 NULL
6760+
2 NULL
6761+
3 1
6762+
4 NULL
6763+
6764+
# All-NULL input — every frame is empty.
6765+
query II
6766+
SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING)
6767+
FROM (VALUES(1,CAST(NULL AS INT)),(2,CAST(NULL AS INT))) t(i,v)
6768+
ORDER BY i;
6769+
----
6770+
1 NULL
6771+
2 NULL
6772+
6773+
# A legitimate 0 must not be turned into NULL, and NULL must follow it.
6774+
# Row 2's frame is [5, 5], whose XOR is genuinely 0. Row 4's frame is
6775+
# [NULL, NULL] and must be NULL even though the accumulated value was already 0.
6776+
# Row 5 shows the accumulator recovering once a non-NULL value re-enters.
6777+
query II
6778+
SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)
6779+
FROM (VALUES(1,5),(2,5),(3,CAST(NULL AS INT)),(4,CAST(NULL AS INT)),(5,3)) t(i,v)
6780+
ORDER BY i;
6781+
----
6782+
1 5
6783+
2 0
6784+
3 5
6785+
4 NULL
6786+
5 3
6787+
67396788
# Covariance/correlation sliding-window regression test. Verifies correct
67406789
# results across row removals and a NULL-gap empty-frame transition.
67416790
query IRRR

0 commit comments

Comments
 (0)