Skip to content

Commit 2876db4

Browse files
committed
fix: error on retracting an untracked value in percentile_cont(DISTINCT)
Per review discussion: if `retract_batch` is asked to remove a value not in the count map, the accumulator's state has diverged from the window frame and continuing would silently produce wrong results. Return an `internal_err!` instead of skipping, so the inconsistency surfaces rather than yielding a wrong answer. Co-authored-by: Claude Code
1 parent 4037b94 commit 2876db4

1 file changed

Lines changed: 15 additions & 6 deletions

File tree

datafusion/functions-aggregate/src/percentile_cont.rs

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -760,21 +760,30 @@ where
760760

761761
let arr = values[0].as_primitive::<T>();
762762
let mut decrement = |value: T::Native| {
763-
if let Some(count) = self.counts.get_mut(&Hashable(value)) {
764-
*count -= 1;
765-
if *count == 0 {
766-
self.counts.remove(&Hashable(value));
763+
match self.counts.get_mut(&Hashable(value)) {
764+
Some(count) => {
765+
*count -= 1;
766+
if *count == 0 {
767+
self.counts.remove(&Hashable(value));
768+
}
769+
Ok(())
767770
}
771+
// Retracting a value that isn't tracked means the accumulator
772+
// state has diverged from the window frame; continuing would
773+
// silently produce wrong results, so surface it as an error.
774+
None => internal_err!(
775+
"percentile_cont(DISTINCT) retract_batch: retracted a value not present in the window"
776+
),
768777
}
769778
};
770779
if arr.null_count() > 0 {
771780
for value in arr.iter().flatten() {
772-
decrement(value);
781+
decrement(value)?;
773782
}
774783
} else {
775784
// Fast path: no nulls, so skip the per-element validity check.
776785
for value in arr.values().iter() {
777-
decrement(*value);
786+
decrement(*value)?;
778787
}
779788
}
780789
Ok(())

0 commit comments

Comments
 (0)