Skip to content

Document and enforce compatibility between an aggregate's accumulators - #25710

Draft
masonh22 wants to merge 9 commits into
apache:mainfrom
coralogix:agg-udf-accum-compat
Draft

masonh22 wants to merge 9 commits into
apache:mainfrom
coralogix:agg-udf-accum-compat

Conversation

@masonh22

@masonh22 masonh22 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

See #25660.

What changes are included in this PR?

This change adds:

  • datafusion_functions_aggregate::testing::check_state_compatibility(): A generic test that identifies cases where the Accumulator and GroupsAccumulator of an AggregateUDF differ. This is exposed as a public function that can be used to test this requirement on AggregateUDFImpls implemented by downstream consumers. The test is run on all built-in default aggregation functions.
  • Fixes for approx_distinct() and avg(), which both violated this requirement
  • Documentation explaining this requirement and how to check for it using the provided test function.

What is the testing strategy for this PR?

Are there any user-facing changes?

@github-actions github-actions Bot added the functions Changes to functions implementation label Sep 24, 2026
@masonh22
masonh22 force-pushed the agg-udf-accum-compat branch from eb9fcb7 to c3496a3 Compare October 5, 2026 17:16
@masonh22
masonh22 force-pushed the agg-udf-accum-compat branch from 99ca719 to 560040a Compare October 5, 2026 17:19
@masonh22
masonh22 force-pushed the agg-udf-accum-compat branch from 6c850b7 to 52043c8 Compare October 5, 2026 17:40
@masonh22 masonh22 changed the title Add a test to enforce compatibility between an aggregate's accumulators Document and enforce compatibility between an aggregate's accumulators Oct 5, 2026
@codecov-commenter

codecov-commenter commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.29878% with 103 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.70%. Comparing base (8248a57) to head (59fe8c5).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...on/functions-aggregate/src/testing/state_compat.rs 85.18% 57 Missing and 31 partials ⚠️
...afusion/functions-aggregate/src/approx_distinct.rs 77.77% 7 Missing and 3 partials ⚠️
datafusion/functions-aggregate/src/average.rs 70.58% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25710      +/-   ##
==========================================
+ Coverage   82.66%   82.70%   +0.03%     
==========================================
  Files        1147     1148       +1     
  Lines      446542   447811    +1269     
  Branches   446542   447811    +1269     
==========================================
+ Hits       369154   370376    +1222     
- Misses      54986    54999      +13     
- Partials    22402    22436      +34     

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

@github-actions github-actions Bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions labels Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation functions Changes to functions implementation logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document and enforce compatibility of the intermediate state produced by an AggregateUDFImpl's Accumulator and GroupsAccumulator

2 participants