refactor: address review feedback on percentile_cont(DISTINCT) accumulator by viirya · Pull Request #23946 · apache/datafusion

viirya · 2026-07-28T15:42:45Z

Rationale for this change

Follow-up to post-merge review feedback from @neilconway on #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.

Follow-up to the DistinctPercentileContAccumulator fix, addressing post-merge review comments: - Use the fast foldhash RandomState for the `counts` map instead of the standard library's default SipHash, matching GenericDistinctBuffer. - Compute size() via estimate_memory_size, mirroring the other distinct accumulators. - Add a null-free fast path to update_batch and retract_batch that skips the per-element validity check when the input array has no nulls. - Add ORDER BY id to the sliding-window regression query in aggregate.slt so its output row order is deterministic. Co-authored-by: Claude Code

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

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

neilconway

Thanks for the quick turnaround! lgtm

viirya · 2026-07-28T16:39:43Z

Thank you @neilconway for the reviews

codecov-commenter · 2026-07-28T19:31:58Z

Codecov Report

❌ Patch coverage is 73.07692% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.70%. Comparing base (994fc81) to head (11cec2d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...afusion/functions-aggregate/src/percentile_cont.rs	73.07%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #23946   +/-   ##
=======================================
  Coverage   80.69%   80.70%           
=======================================
  Files        1095     1095           
  Lines      372626   372542   -84     
  Branches   372626   372542   -84     
=======================================
- Hits       300700   300651   -49     
+ Misses      53978    53924   -54     
- Partials    17948    17967   +19

☔ 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.

Add a sliding-window percentile_cont(DISTINCT ...) regression over data containing a NULL positioned so that both the update_batch (NULL row enters the frame) and retract_batch (NULL row leaves the frame) code paths take the null_count() > 0 branch. Verified via temporary eprintln that both branches execute; the previous distinct-percentile tests used no NULLs and left those branches uncovered. Co-authored-by: Claude Code

…edian` (apache#23954) ## Which issue does this PR close? - Closes apache#23953 ## Rationale for this change This PR makes three improvements to non-distinct `PercentileContAccumulator` and `MedianAccumulator`, inspired by recent work on `percentile_cont(DISTINCT)` (apache#23946): 1. Switch from SipHash to foldhash for internal hash maps 2. Add a null-free fast path to `update_batch` and `retract_batch` 3. Raise an error if we attempt to retract an unknown value in `retract_batch`, rather than silently ignoring it. Benchmarks: percentile_cont no_nulls window=256 -61.1% (249.0 -> 96.6 us) percentile_cont with_nulls window=256 -59.4% (232.7 -> 94.5 us) percentile_cont no_nulls window=4096 -50.9% (756.3 -> 370.4 us) percentile_cont with_nulls window=4096 -48.2% (552.2 -> 286.2 us) percentile_cont no_nulls window=16384 -47.3% (2.311 -> 1.218 ms) percentile_cont with_nulls window=16384 -42.0% (1.465 -> 0.851 ms) median no_nulls window=256 -62.8% (247.0 -> 92.0 us) median with_nulls window=256 -59.8% (228.8 -> 92.0 us) median no_nulls window=4096 -40.0% (411.4 -> 246.6 us) median with_nulls window=4096 -39.4% (364.4 -> 221.0 us) median no_nulls window=16384 -31.5% (1.107 -> 0.759 ms) median with_nulls window=16384 -32.9% (0.947 -> 0.637 ms) ## What changes are included in this PR? See above. ## Are these changes tested? Yes: existing tests pass, new tests added for the null-free fast path and the improved error handling. ## Are there any user-facing changes? No.

@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.

…edian` (apache#23954) ## Which issue does this PR close? - Closes apache#23953 ## Rationale for this change This PR makes three improvements to non-distinct `PercentileContAccumulator` and `MedianAccumulator`, inspired by recent work on `percentile_cont(DISTINCT)` (apache#23946): 1. Switch from SipHash to foldhash for internal hash maps 2. Add a null-free fast path to `update_batch` and `retract_batch` 3. Raise an error if we attempt to retract an unknown value in `retract_batch`, rather than silently ignoring it. Benchmarks: percentile_cont no_nulls window=256 -61.1% (249.0 -> 96.6 us) percentile_cont with_nulls window=256 -59.4% (232.7 -> 94.5 us) percentile_cont no_nulls window=4096 -50.9% (756.3 -> 370.4 us) percentile_cont with_nulls window=4096 -48.2% (552.2 -> 286.2 us) percentile_cont no_nulls window=16384 -47.3% (2.311 -> 1.218 ms) percentile_cont with_nulls window=16384 -42.0% (1.465 -> 0.851 ms) median no_nulls window=256 -62.8% (247.0 -> 92.0 us) median with_nulls window=256 -59.8% (228.8 -> 92.0 us) median no_nulls window=4096 -40.0% (411.4 -> 246.6 us) median with_nulls window=4096 -39.4% (364.4 -> 221.0 us) median no_nulls window=16384 -31.5% (1.107 -> 0.759 ms) median with_nulls window=16384 -32.9% (0.947 -> 0.637 ms) ## What changes are included in this PR? See above. ## Are these changes tested? Yes: existing tests pass, new tests added for the null-free fast path and the improved error handling. ## Are there any user-facing changes? No.

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

viirya added 2 commits July 28, 2026 08:55

viirya mentioned this pull request Jul 28, 2026

fix: correct percentile_cont(DISTINCT) accumulation and sliding-window retract #23913

Merged

neilconway approved these changes Jul 28, 2026

View reviewed changes

This was referenced Jul 28, 2026

Optimize hashing, null-free fast path in percentile_cont, median #23953

Closed

perf: Optimize hashing, null-free fast path for percentile_cont, median #23954

Merged

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

Merged via the queue into apache:main with commit 73b8ad3 Jul 28, 2026
37 checks passed

viirya deleted the followup-distinct-percentile branch July 28, 2026 22:37

neilconway mentioned this pull request Jul 29, 2026

chore: refactor VarianceAccumulator, add tests and benchmark #23977

Merged

Navigation Menu

Uh oh!

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

refactor: address review feedback on percentile_cont(DISTINCT) accumulator#23946
viirya merged 4 commits into
apache:mainfrom
viirya:followup-distinct-percentile

viirya commented Jul 28, 2026

Uh oh!

neilconway left a comment

Uh oh!

viirya commented Jul 28, 2026

Uh oh!

codecov-commenter commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

viirya commented Jul 28, 2026

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

neilconway left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Jul 28, 2026

Uh oh!

codecov-commenter commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 28, 2026 •

edited

Loading