fix: correct percentile_cont(DISTINCT) accumulation and sliding-window retract by viirya · Pull Request #23913 · apache/datafusion

viirya · 2026-07-27T00:40:47Z

Which issue does this PR close?

Closes percentile_cont(DISTINCT ...) panics on every query, and returns wrong results in sliding windows #23912.

Rationale for this change

DistinctPercentileContAccumulator reused the shared set-based GenericDistinctBuffer, which doesn't fit it: (1) the buffer asserts a single input column but percentile_cont passes two (value + percentile), so every percentile_cont(DISTINCT ...) panicked; (2) the buffer is a plain HashSet with no multiplicity, so sliding-window retract_batch dropped a value while duplicates were still in the frame.

What changes are included in this PR?

Replace the shared buffer in this accumulator with a per-accumulator HashMap<Hashable, usize> count map: update_batch reads only the value column and increments; retract_batch decrements and removes a key only at zero; state/merge_batch keep the same List state shape. Other GenericDistinctBuffer users are untouched.
Regression tests in aggregate.slt for the plain distinct query and the sliding-window duplicate-retract case.

Are these changes tested?

Yes — new regression tests; the full aggregate.slt suite passes.

Are there any user-facing changes?

percentile_cont(DISTINCT ...) now works instead of panicking, and returns correct results in sliding windows.

…w retract `DistinctPercentileContAccumulator` had two bugs: 1. Panic on every distinct query. `update_batch` forwarded all argument columns to `GenericDistinctBuffer::update_batch`, which asserts a single input array. `percentile_cont` always passes two columns (the value and the percentile literal), so any `percentile_cont(DISTINCT x, p)` panicked with "DistinctValuesBuffer::update_batch expects only a single input array". 2. Wrong results in sliding windows. The buffer was a plain `HashSet` with no per-value multiplicity, so `retract_batch` removed a value entirely when a single occurrence left the window frame, even if other rows in the frame still carried that value. Replace the shared set-based buffer with a per-accumulator `HashMap<Hashable, usize>` count map: `update_batch` reads only the value column and increments counts, `retract_batch` decrements and removes a key only at zero, and `state`/`merge_batch` serialize the distinct keys as a List (unchanged state shape). The other `GenericDistinctBuffer` users (count/sum/ median/variance distinct) are unaffected; only percentile_cont supports retract, so it is the sole accumulator that needed multiplicity tracking. Add regression coverage in aggregate.slt for the plain distinct aggregate and for the sliding-window duplicate-value retract case. Co-authored-by: Claude Code

codecov-commenter · 2026-07-27T01:09:25Z

Codecov Report

❌ Patch coverage is 41.37931% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.67%. Comparing base (77b172e) to head (3499659).

Files with missing lines	Patch %	Lines
...afusion/functions-aggregate/src/percentile_cont.rs	41.37%	15 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #23913   +/-   ##
=======================================
  Coverage   80.67%   80.67%           
=======================================
  Files        1094     1094           
  Lines      371770   371790   +20     
  Branches   371770   371790   +20     
=======================================
+ Hits       299907   299930   +23     
+ Misses      53964    53954   -10     
- Partials    17899    17906    +7

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

viirya · 2026-07-28T02:51:46Z

Thanks @Jefffrey for review

neilconway · 2026-07-28T14:17:31Z

 struct DistinctPercentileContAccumulator<T: ArrowNumericType> {
-    distinct_values: GenericDistinctBuffer<T>,
+    /// Distinct value -> number of in-window rows carrying it.
+    counts: HashMap<Hashable<T::Native>, usize>,


GenericDistinctBuffer previously used foldhash, but this will switch to using siphash, which is a lot slower.

Good catch — switched the count map to the foldhash RandomState that GenericDistinctBuffer uses, instead of the std default SipHash. Fixed in #23946.

neilconway · 2026-07-28T14:19:50Z

+SELECT id, percentile_cont(DISTINCT x, 0.5)
+  OVER (ORDER BY id ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)
+FROM distinct_pct;


This needs an ORDER BY to be deterministic, or else use rowsort SLT directive.

Added ORDER BY id to make the row order deterministic. Fixed in #23946.

neilconway · 2026-07-28T14:25:52Z

-        size_of_val(self) + self.distinct_values.size()
+        size_of_val(self)
+            + self.counts.capacity()
+                * (size_of::<Hashable<T::Native>>() + size_of::<usize>())


estimate_memory_size::<(Hashable<T::Native>, usize)>(self.counts.capacity(), size_of_val(self)) is better I think.

Switched to estimate_memory_size::<(Hashable<T::Native>, usize)>(self.counts.capacity(), size_of_val(self)). Fixed in #23946.

neilconway · 2026-07-28T14:27:37Z

+            if let Some(count) = self.counts.get_mut(&Hashable(value)) {
+                *count -= 1;
+                if *count == 0 {
+                    self.counts.remove(&Hashable(value));
+                }
+            }


wdyt of raising an error if the caller attempts to retract a value that isn't in the map?

I left retract_batch silently skipping a value that isn't in the map rather than erroring. The sliding-window executor only retracts values it previously passed to update_batch, so a missing key shouldn't happen in practice — but if it ever did (a bug elsewhere), erroring would turn a latent inconsistency into a hard query failure, whereas skipping keeps the window's remaining counts intact. I'm not strongly attached either way; if you'd prefer a debug_assert! (catches it in tests/debug builds without failing production queries), I'm happy to add that instead. What do you think?

If we try to retract a missing value, seems that we are likely to eventually produce incorrect query results, so I'd argue it is better to raise an error here rather than to silently proceed and return the wrong answer. I'd personally argue for internal_err!; but if you disagree, then I'd say debug_assert! is better than nothing 😊

Agreed — returning a wrong answer is worse than failing loudly here. Went with internal_err! in #23946: retracting a value not in the count map now surfaces as an error rather than silently proceeding. Thanks!

neilconway · 2026-07-28T14:30:03Z

+        let arr = values[0].as_primitive::<T>();
+        for value in arr.iter().flatten() {
+            *self.counts.entry(Hashable(value)).or_default() += 1;
+        }


GenericDistinctBuffer has a fast-path for null-free arrays; might be worth adopting here as well.

Adopted the same null-free fast path in update_batch and retract_batch. Fixed in #23946.

neilconway · 2026-07-28T14:31:02Z

Btw I notice that CodeCov says the test coverage is only 41%, which seems low?

Add a grouped percentile_cont(DISTINCT ...) regression test. A GROUP BY with target_partitions=4 forces two-phase (Partial + FinalPartitioned) aggregation, so the distinct accumulator's state() and merge_batch() paths run — the paths the previous whole-table regression queries did not reach (flagged by patch-coverage review on apache#23913). Duplicate values within each group also verify multiplicity is preserved across the per-partition merge. Co-authored-by: Claude Code

viirya · 2026-07-28T16:00:04Z

On the coverage: the uncovered lines were mostly state() / merge_batch(), which only run under two-phase (Partial → Final) aggregation. The regression queries here are whole-table aggregates that don't trigger that, so those paths showed as uncovered even though the core update / retract / evaluate logic is exercised.

I've added a grouped percentile_cont(DISTINCT ...) test in #23946 that forces two-phase aggregation (GROUP BY with rows repartitioned across partitions — confirmed via EXPLAIN it plans AggregateExec: mode=Partial → RepartitionExec: Hash → mode=FinalPartitioned), so state() and merge_batch() are now covered. It uses duplicate values within each group, which also verifies the count-map de-dups correctly across the partial-state merge.

@neilconway

…lator (apache#23946) ## Rationale for this change Follow-up to post-merge review feedback from @neilconway on apache#23913. ## What changes are included in this PR? - Use the fast foldhash `RandomState` for the distinct-value count map instead of the standard library's default SipHash (the shared `GenericDistinctBuffer` already does this; the merged fix regressed to SipHash on this hot path). - Use `estimate_memory_size` in `size()` instead of a hand-rolled capacity calculation. - Adopt the null-free fast path in `update_batch`/`retract_batch` (skip per-element validity checks when the input has no nulls), mirroring `GenericDistinctBuffer`. - Add `ORDER BY` to the sliding-window regression test so its row order is deterministic. ## Are these changes tested? Yes — existing percentile unit tests and the full `aggregate.slt` pass. ## Are there any user-facing changes? No.

github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Jul 27, 2026

Jefffrey approved these changes Jul 28, 2026

View reviewed changes

viirya added this pull request to the merge queue Jul 28, 2026

Merged via the queue into apache:main with commit 0b07e58 Jul 28, 2026
61 of 62 checks passed

viirya deleted the fix-distinct-percentile-retract branch July 28, 2026 03:03

neilconway reviewed Jul 28, 2026

View reviewed changes

viirya mentioned this pull request Jul 28, 2026

refactor: address review feedback on percentile_cont(DISTINCT) accumulator #23946

Merged

Uh oh!

Conversation

viirya commented Jul 27, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Jul 27, 2026

Codecov Report

Uh oh!

viirya commented Jul 28, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neilconway commented Jul 28, 2026

Uh oh!

viirya commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants