You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/20 11:39:25 UTC
(datafusion) 01/01: fix: return NULL from `bit_xor` when a sliding frame has no non-NULL values (#26485)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-26485-2c2ecae08bd49b32f97aef2ff888742057f37ac6
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit cee411484627dd2d1123d8580de8d00862f2ca7b
Author: Cheng-Yi Shih <48...@users.noreply.github.com>
AuthorDate: Tue Oct 20 11:39:00 2026 +0000
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 <no...@anthropic.com>
---
.../functions-aggregate/src/bit_and_or_xor.rs | 77 +++++++++++++++++++---
datafusion/sqllogictest/test_files/window.slt | 49 ++++++++++++++
2 files changed, 118 insertions(+), 8 deletions(-)
diff --git a/datafusion/functions-aggregate/src/bit_and_or_xor.rs b/datafusion/functions-aggregate/src/bit_and_or_xor.rs
index 1928f5c51c..3353d958f3 100644
--- a/datafusion/functions-aggregate/src/bit_and_or_xor.rs
+++ b/datafusion/functions-aggregate/src/bit_and_or_xor.rs
@@ -433,7 +433,11 @@ where
}
struct BitXorAccumulator<T: ArrowNumericType> {
- value: Option<T::Native>,
+ /// Running XOR of every value seen so far, starting from the identity `0`.
+ /// Only meaningful when `non_null_count` is non-zero.
+ value: T::Native,
+ /// Number of non-NULL values currently accumulated (inserted minus retracted)
+ non_null_count: usize,
}
impl<T: ArrowNumericType> std::fmt::Debug for BitXorAccumulator<T> {
@@ -444,7 +448,10 @@ impl<T: ArrowNumericType> std::fmt::Debug for BitXorAccumulator<T> {
impl<T: ArrowNumericType> Default for BitXorAccumulator<T> {
fn default() -> Self {
- Self { value: None }
+ Self {
+ value: T::Native::usize_as(0),
+ non_null_count: 0,
+ }
}
}
@@ -454,15 +461,20 @@ where
{
fn update_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
if let Some(x) = arrow::compute::bit_xor(values[0].as_primitive::<T>()) {
- let v = self.value.get_or_insert_with(|| T::Native::usize_as(0));
- *v = *v ^ x;
+ self.value = self.value ^ x;
}
+ self.non_null_count += values[0].len() - values[0].null_count();
Ok(())
}
fn retract_batch(&mut self, values: &[ArrayRef]) -> Result<()> {
- // XOR is it's own inverse
- self.update_batch(values)
+ // XOR is its own inverse, so the value is updated as in `update_batch`;
+ // only the count of non-NULL values moves the other way
+ if let Some(x) = arrow::compute::bit_xor(values[0].as_primitive::<T>()) {
+ self.value = self.value ^ x;
+ }
+ self.non_null_count -= values[0].len() - values[0].null_count();
+ Ok(())
}
fn supports_retract_batch(&self) -> bool {
@@ -470,7 +482,9 @@ where
}
fn evaluate(&mut self) -> Result<ScalarValue> {
- ScalarValue::new_primitive::<T>(self.value, &T::DATA_TYPE)
+ // All non-NULL values may have been retracted, in which case the result is NULL
+ let value = (self.non_null_count > 0).then_some(self.value);
+ ScalarValue::new_primitive::<T>(value, &T::DATA_TYPE)
}
fn size(&self) -> usize {
@@ -580,7 +594,7 @@ mod tests {
#[test]
fn test_bit_xor_accumulator() {
- let mut accumulator = BitXorAccumulator::<UInt64Type> { value: None };
+ let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
let batches: Vec<_> = vec![vec![1, 2], vec![1]]
.into_iter()
.map(|b| Arc::new(b.into_iter().collect::<UInt64Array>()) as ArrayRef)
@@ -603,4 +617,51 @@ mod tests {
ScalarValue::UInt64(Some(2))
);
}
+
+ #[test]
+ fn test_bit_xor_accumulator_retract_to_all_null() {
+ let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
+ let five = [Arc::new(UInt64Array::from(vec![Some(5)])) as ArrayRef];
+ let nulls = [Arc::new(UInt64Array::from(vec![None])) as ArrayRef];
+ let seven = [Arc::new(UInt64Array::from(vec![Some(7)])) as ArrayRef];
+
+ accumulator.update_batch(&five).unwrap();
+ assert_eq!(
+ accumulator.evaluate().unwrap(),
+ ScalarValue::UInt64(Some(5))
+ );
+
+ accumulator.update_batch(&nulls).unwrap();
+ assert_eq!(
+ accumulator.evaluate().unwrap(),
+ ScalarValue::UInt64(Some(5))
+ );
+
+ // All non-NULL values left the frame, so the result is NULL
+ accumulator.retract_batch(&five).unwrap();
+ assert_eq!(accumulator.evaluate().unwrap(), ScalarValue::UInt64(None));
+
+ // Retracting NULLs must not move the count
+ accumulator.retract_batch(&nulls).unwrap();
+ assert_eq!(accumulator.evaluate().unwrap(), ScalarValue::UInt64(None));
+
+ accumulator.update_batch(&seven).unwrap();
+ assert_eq!(
+ accumulator.evaluate().unwrap(),
+ ScalarValue::UInt64(Some(7))
+ );
+ }
+
+ #[test]
+ fn test_bit_xor_accumulator_legitimate_zero() {
+ let mut accumulator = BitXorAccumulator::<UInt64Type>::default();
+ let values = [Arc::new(UInt64Array::from(vec![Some(5), Some(5)])) as ArrayRef];
+
+ // 5 ^ 5 is a real zero, not NULL
+ accumulator.update_batch(&values).unwrap();
+ assert_eq!(
+ accumulator.evaluate().unwrap(),
+ ScalarValue::UInt64(Some(0))
+ );
+ }
}
diff --git a/datafusion/sqllogictest/test_files/window.slt b/datafusion/sqllogictest/test_files/window.slt
index 7c92514e72..cea8060c13 100644
--- a/datafusion/sqllogictest/test_files/window.slt
+++ b/datafusion/sqllogictest/test_files/window.slt
@@ -6736,6 +6736,55 @@ ORDER BY i;
3 1
4 NULL
+# bit_xor over a sliding window must yield NULL when the frame has no non-NULL
+# values, matching the result over the whole partition (apache/datafusion#26470).
+# Before the fix the sliding result was 0 on the NULL row.
+query III
+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, CAST(NULL AS INT)), (3, 6)) t(i, x)
+ORDER BY i;
+----
+5 5 5
+NULL NULL NULL
+6 6 6
+
+# Narrow sliding frame that drains to empty each row.
+query II
+SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING)
+FROM (VALUES(1,CAST(NULL AS INT)),(2,1),(3,CAST(NULL AS INT)),(4,CAST(NULL AS INT))) t(i,v)
+ORDER BY i;
+----
+1 NULL
+2 NULL
+3 1
+4 NULL
+
+# All-NULL input — every frame is empty.
+query II
+SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING)
+FROM (VALUES(1,CAST(NULL AS INT)),(2,CAST(NULL AS INT))) t(i,v)
+ORDER BY i;
+----
+1 NULL
+2 NULL
+
+# A legitimate 0 must not be turned into NULL, and NULL must follow it.
+# Row 2's frame is [5, 5], whose XOR is genuinely 0. Row 4's frame is
+# [NULL, NULL] and must be NULL even though the accumulated value was already 0.
+# Row 5 shows the accumulator recovering once a non-NULL value re-enters.
+query II
+SELECT i, bit_xor(v) OVER (ORDER BY i ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)
+FROM (VALUES(1,5),(2,5),(3,CAST(NULL AS INT)),(4,CAST(NULL AS INT)),(5,3)) t(i,v)
+ORDER BY i;
+----
+1 5
+2 0
+3 5
+4 NULL
+5 3
+
# Covariance/correlation sliding-window regression test. Verifies correct
# results across row removals and a NULL-gap empty-frame transition.
query IRRR
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org