feat: add row-count-aware accumulator interfaces - #25886
wudidapaopao wants to merge 4 commits into
Conversation
0e4697b to
055d3b2
Compare
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
mhilton
left a comment
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
I wonder if using true_count() would be mildly speedier here.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
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?
GroupsAccumulatorAdapterto pass per-group row counts, including FILTER and state conversion.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.