alamb opened a new pull request, #24728:
URL: https://github.com/apache/datafusion/pull/24728
## Which issue does this PR close?
- Part of #24727
## Rationale for this change
The `udaf_default_*` display / schema name helpers in
`datafusion/expr/src/udaf.rs` are generic over `<F: AggregateUDFImpl + ?Sized>`
but only use `F` to call `name()` (and `supports_within_group_clause()`), so
every `AggregateUDFImpl` implementation gets its own identical copy of each
helper body. This is an example PR for "pattern 1" from #24727 (generics that
only call trait methods): pass the values the helper actually needs so the body
is compiled exactly once.
## What changes are included in this PR?
1. Change the five helpers to be non-generic, taking the function name (and
for `udaf_default_schema_name`, the `supports_within_group_clause` flag)
instead of `&F`:
- `udaf_default_schema_name`
- `udaf_default_human_display`
- `udaf_default_window_function_schema_name`
- `udaf_default_display_name`
- `udaf_default_window_function_display_name`
2. Update the `AggregateUDFImpl` default methods to extract those values and
call the shared function, and update the trait documentation to mention the
default implementation for each method
3. Add an entry to the 56.0.0 upgrade guide (the helpers were released in
51.0.0, so this is an API change)
`udaf_default_return_field` is deliberately left alone: it is not a display
/ schema name helper and most of its body is the (necessarily polymorphic)
`return_type` call.
Note: the default trait methods can't simply take `&dyn AggregateUDFImpl`
because `Self` is `?Sized` in the default method body, so `self` can't be
coerced to a trait object there. Passing the extracted values sidesteps that
and generates the same amount of per-impl code (two accessor calls + one direct
call).
## Code size
Measured per the methodology in #24727, at merge base a38bb10c33 and on this
branch.
`cargo llvm-lines --release -p datafusion-functions-aggregate --lib` (29
`AggregateUDFImpl` implementations):
| | main (a38bb10c33) | this branch
| change |
| ------------------------------ | ---------------------- |
----------------- | ---------------------- |
| `udaf_default_*` display/schema name helpers | 212,048 IR lines (2,755
functions) | 0 | **-212,048 (-100%)** |
| Crate total | 2,112,337 IR lines (39,704 copies) |
1,899,702 IR lines (36,936 copies) | **-212,635 (-10.1%)** |
`cargo llvm-lines --release -p datafusion-expr --lib` (the helpers now
compile here exactly once):
| | main (a38bb10c33) | this branch | change
|
| ----------- | ------------------ | ------------------ |
------------------- |
| Crate total | 1,265,577 IR lines (26,405 copies) | 1,229,049 IR lines
(25,924 copies) | **-36,528 (-2.9%)** |
`datafusion-cli` release binary size comparison to follow.
## Are these changes tested?
Covered by existing tests (`cargo test -p datafusion-expr -p
datafusion-functions-aggregate`); this is a pure refactor of where the
default-implementation bodies live, with no behavior change.
## Are there any user-facing changes?
Yes: the signatures of the five public `udaf_default_*` helpers changed
(they now take `name: &str` etc. instead of a generic `AggregateUDFImpl`
reference). The default `AggregateUDFImpl` trait methods behave identically.
Documented in the 56.0.0 upgrade guide; this PR should get the `api change`
label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
