Skip to content

chore: refactor VarianceAccumulator, add tests and benchmark - #23977

Merged
neilconway merged 1 commit into
apache:mainfrom
neilconway:neilc/chore-variance-cleanups
Jul 29, 2026
Merged

chore: refactor VarianceAccumulator, add tests and benchmark#23977
neilconway merged 1 commit into
apache:mainfrom
neilconway:neilc/chore-variance-cleanups

Conversation

@neilconway

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • N/A

Rationale for this change

This PR adds a benchmark for VarianceAccumulator::update_batch, and retract_batch, adds some unit tests, and refactors the retract_batch code to add a helper.

What changes are included in this PR?

See above.

Are these changes tested?

Yes, new tests added.

Are there any user-facing changes?

No.

…elper

Adds a criterion benchmark covering VarianceAccumulator::update_batch
and retract_batch on null-free and 10%-null f64 batches, and unit tests
covering null handling for update, retract, and retract underflow.
Extracts the retract_batch loop body into a `retract` free function
alongside the existing `update`.

A null-free fast path for update_batch/retract_batch (iterating the
values buffer directly when null_count() == 0, as COUNT(DISTINCT) does
after apache#23956 and percentile_cont/median after apache#23954) was evaluated and
rejected: back-to-back runs of the new benchmark show it is within noise
(-0.3% to +1.6%), because the Welford update is a serial floating-point
dependency chain whose latency hides the per-element validity check.
@github-actions github-actions Bot added the functions Changes to functions implementation label Jul 29, 2026
@neilconway

Copy link
Copy Markdown
Contributor Author

FYI: I explored whether adding a null-free fast path to VarianceAccumulator::update_batch and retract_batch would be a win, following recent work on other accumulators (#23956, #23954, #23946). In this case, it didn't seem to be a significant win, probably because of the floating-point intensive work we do inside the accumulator itself, but the associated benchmarks and test improvements still seem useful.

cc @viirya

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The retract helper is a byte-for-byte extraction of the previous inline Welford inverse — same count <= 1 reset, same delta/mean/m2 recurrence — and it reads better paired with the existing update free function.

The tests are a good addition, especially since they pin down the NULL and underflow behavior that retract_batch relies on:

  • update_batch_ignores_nulls / retract_batch_ignores_nulls — dense vs. sparse equivalence (I verified the expected sample variances: 5/3 for {1,2,3,4}, 0.5 for the remaining {3,4}).
  • retract_batch_resets_when_underflowing — nicely covers the count <= 1 → empty reset for both dense and null-containing retract batches, which is exactly the branch that keeps variance correct when a sliding window drains.

Benchmark shape looks right too (the iter_batched retract setup accumulates two batches so retracting one leaves rows, mirroring a sliding window). LGTM.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.75%. Comparing base (043d97f) to head (d35a105).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-aggregate/src/variance.rs 71.42% 0 Missing and 18 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23977      +/-   ##
==========================================
- Coverage   80.75%   80.75%   -0.01%     
==========================================
  Files        1096     1096              
  Lines      373506   373582      +76     
  Branches   373506   373582      +76     
==========================================
+ Hits       301629   301677      +48     
- Misses      53893    53896       +3     
- Partials    17984    18009      +25     

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

@neilconway
neilconway added this pull request to the merge queue Jul 29, 2026
Merged via the queue into apache:main with commit 1955d5a Jul 29, 2026
38 checks passed
@neilconway
neilconway deleted the neilc/chore-variance-cleanups branch July 29, 2026 16:26
@neilconway

Copy link
Copy Markdown
Contributor Author

@viirya Thanks for the review!

kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
…e#23977)

## Which issue does this PR close?

- N/A

## Rationale for this change

This PR adds a benchmark for `VarianceAccumulator::update_batch`, and
`retract_batch`, adds some unit tests, and refactors the `retract_batch`
code to add a helper.

## What changes are included in this PR?

See above.

## Are these changes tested?

Yes, new tests added.

## Are there any user-facing changes?

No.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants