Skip to content

feat: add row-count-aware accumulator interfaces - #25886

Open
wudidapaopao wants to merge 4 commits into
apache:mainfrom
wudidapaopao:inputless-aggregate-interfaces
Open

wudidapaopao wants to merge 4 commits into
apache:mainfrom
wudidapaopao:inputless-aggregate-interfaces

Conversation

@wudidapaopao

@wudidapaopao wudidapaopao commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Aggregate UDFs without arguments receive an empty array slice, so accumulators cannot derive the input row count from their arguments.

What changes are included in this PR?

  • Adds argument structs and default accumulator methods that carry evaluated arguments with an explicit row count.
  • Adapts GroupsAccumulatorAdapter to pass per-group row counts, including FILTER and state conversion.
  • Propagates row counts through physical aggregate execution while preserving existing UDAF behavior.
  • This PR supports native Rust UDAFs; forwarding the new row-count context across FFI is left for follow-up work.

What is the testing strategy for this PR?

Added tests for ungrouped inputless aggregation, grouped Adapter execution, FILTER handling, empty state conversion, partial-skip, nullary signature validation, and the unsupported inputless window path.

Are there any user-facing changes?

Adds optional, defaulted public accumulator methods. Existing UDAF implementations remain source-compatible.

@github-actions github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates functions Changes to functions implementation physical-plan Changes to the physical-plan crate labels Sep 29, 2026
@github-actions github-actions Bot added the auto detected api change Auto detected API change label Sep 29, 2026
@wudidapaopao wudidapaopao changed the title feat: support inputless aggregate UDFs feat: add row-count-aware accumulator interfaces Sep 29, 2026
@wudidapaopao
wudidapaopao force-pushed the inputless-aggregate-interfaces branch from 0e4697b to 055d3b2 Compare September 29, 2026 21:11
@github-actions github-actions Bot removed the auto detected api change Auto detected API change label Sep 29, 2026
@codecov-commenter

codecov-commenter commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.54292% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.63%. Comparing base (b9c8b3d) to head (fd65f12).
⚠️ Report is 54 commits behind head on main.

Files with missing lines Patch % Lines
...gregate-common/src/aggregate/groups_accumulator.rs 84.77% 23 Missing and 7 partials ⚠️
datafusion/physical-plan/src/aggregates/mod.rs 80.00% 22 Missing and 5 partials ⚠️
...n/physical-plan/src/aggregates/aggregate_stream.rs 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25886      +/-   ##
==========================================
+ Coverage   82.57%   82.63%   +0.05%     
==========================================
  Files        1142     1147       +5     
  Lines      441215   445700    +4485     
  Branches   441215   445700    +4485     
==========================================
+ Hits       364332   368282    +3950     
- Misses      54841    55053     +212     
- Partials    22042    22365     +323     

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

@mhilton mhilton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for breaking this PR out. It makes sense to me and I prefer the interface you've added here.

opt_filter.map_or(length, |filter| {
filter
.slice(offset, length)
.iter()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if using true_count() would be mildly speedier here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Updated to use true_count().

WindowFunctionDefinition::AggregateUDF(fun) => {
if args.is_empty() {
return not_impl_err!(
"Aggregate window function {} without arguments is not supported",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suppose this does technically invalidate https://datafusion.apache.org/user-guide/sql/window_functions.html#aggregate-functions. I'm not sure that is enough to require making this work in this PR though.

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

Labels

functions Changes to functions implementation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support inputless aggregate UDFs with explicit row counts

3 participants