You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/08/27 14:37:58 UTC
[PR] Make `udaf_default_*` display/schema name helpers non-generic to reduce code size [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Make `udaf_default_*` display/schema name helpers non-generic (214 KB smaller binary) [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #24728:
URL: https://github.com/apache/datafusion/pull/24728
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Make `udaf_default_*` display/schema name helpers non-generic (214 KB smaller binary) [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24728:
URL: https://github.com/apache/datafusion/pull/24728#issuecomment-5442638249
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24728?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `79.39698%` with `41 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.44%. Comparing base ([`d66f4b9`](https://app.codecov.io/gh/apache/datafusion/commit/d66f4b97a5fedd2dc2404edaf3f7a5c8a6b386d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`59a4294`](https://app.codecov.io/gh/apache/datafusion/commit/59a4294901e8c1763114d63f28b37eba824f445a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24728?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/udaf.rs](https://app.codecov.io/gh/apache/datafusion/pull/24728?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Fudaf.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy91ZGFmLnJz) | 79.39% | [21 Missing and 20 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24728?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24728 +/- ##
==========================================
- Coverage 81.45% 81.44% -0.01%
==========================================
Files 1120 1120
Lines 401793 401838 +45
Branches 401793 401838 +45
==========================================
+ Hits 327262 327274 +12
- Misses 55356 55385 +29
- Partials 19175 19179 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24728?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Make `udaf_default_*` display/schema name helpers non-generic (214 KB smaller binary) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24728:
URL: https://github.com/apache/datafusion/pull/24728#issuecomment-5442991896
Extended teset failure seems to be failing on main too
```
joins::hash_join::exec::tests::test_partitioned_null_equal_dynamic_filter_keeps_probe_nulls_for_build_logical_null
```
Here is a ticket
- https://github.com/apache/datafusion/issues/24734
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Make `udaf_default_*` display/schema name helpers non-generic to reduce code size [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24728:
URL: https://github.com/apache/datafusion/pull/24728#issuecomment-5441190845
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-expr v55.0.0 (current)
Built [ 31.639s] (current)
Parsing datafusion-expr v55.0.0 (current)
Parsed [ 0.076s] (current)
Building datafusion-expr v55.0.0 (baseline)
Built [ 30.736s] (baseline)
Parsing datafusion-expr v55.0.0 (baseline)
Parsed [ 0.077s] (baseline)
Checking datafusion-expr v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.385s] 223 checks: 221 pass, 2 fail, 0 warn, 31 skip
--- failure function_parameter_count_changed: pub fn parameter count changed ---
Description:
A publicly-visible function now takes a different number of parameters.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_parameter_count_changed.ron
Failed in:
datafusion_expr::udaf_default_schema_name now takes 3 parameters instead of 2, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:965
--- failure function_requires_different_generic_type_params: function now requires a different number of generic type parameters ---
Description:
A function now requires a different number of generic type parameters than it used to. Uses of this function that supplied the previous number of generic types (e.g. via turbofish syntax) will be broken.
ref: https://doc.rust-lang.org/reference/items/generics.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_requires_different_generic_type_params.ron
Failed in:
function udaf_default_window_function_schema_name (1 -> 0 generic types) in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:1072
function udaf_default_schema_name (1 -> 0 generic types) in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:965
function udaf_default_display_name (1 -> 0 generic types) in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:1137
function udaf_default_human_display (1 -> 0 generic types) in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:1026
function udaf_default_window_function_display_name (1 -> 0 generic types) in /home/runner/work/datafusion/datafusion/datafusion/expr/src/udaf.rs:1185
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 65.186s] datafusion-expr
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Make `udaf_default_*` display/schema name helpers non-generic (214 KB smaller binary) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24728:
URL: https://github.com/apache/datafusion/pull/24728#discussion_r3873346597
##########
datafusion/expr/src/udaf.rs:
##########
@@ -943,258 +955,498 @@ impl PartialOrd for dyn AggregateUDFImpl {
}
}
+/// Builds the default implementation of [`AggregateUDFImpl::schema_name`].
Review Comment:
Here is a new builder for schema name -- it is basically the same pattern for all of the other types
##########
datafusion/expr/src/udaf.rs:
##########
@@ -459,15 +459,21 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any {
/// See [`Expr::schema_name`] for details
///
/// Example of schema_name: count(DISTINCT column1) FILTER (WHERE column2 > 10) ORDER BY [..]
+ ///
+ /// The default implementation is provided by [`UdafSchemaNameBuilder`]
fn schema_name(&self, params: &AggregateFunctionParams) -> Result<String> {
- udaf_default_schema_name(self, params)
+ UdafSchemaNameBuilder::new(self.name(), params)
Review Comment:
The core rationale is to use a single non generic function -- I also made a struct like builder to make the code easier to see too
##########
datafusion/expr/src/udaf.rs:
##########
@@ -943,258 +955,498 @@ impl PartialOrd for dyn AggregateUDFImpl {
}
}
+/// Builds the default implementation of [`AggregateUDFImpl::schema_name`].
+///
+/// # Example
+/// ```
+/// # use datafusion_expr::col;
+/// # use datafusion_expr::expr::{AggregateFunctionParams, Sort};
+/// # use datafusion_expr::UdafSchemaNameBuilder;
+/// # let params = AggregateFunctionParams {
+/// # args: vec![col("x")],
+/// # distinct: true,
+/// # filter: None,
+/// # order_by: vec![Sort::new(col("y"), true, false)],
+/// # null_treatment: None,
+/// # };
+/// let schema_name = UdafSchemaNameBuilder::new("my_udaf", ¶ms)
+/// .with_supports_within_group_clause(false)
+/// .build()
+/// .unwrap();
+/// assert_eq!(schema_name, "my_udaf(DISTINCT x) ORDER BY [y ASC NULLS LAST]");
+/// ```
+#[derive(Debug)]
+pub struct UdafSchemaNameBuilder<'a> {
+ name: &'a str,
+ params: &'a AggregateFunctionParams,
+ supports_within_group_clause: bool,
+}
+
+impl<'a> UdafSchemaNameBuilder<'a> {
+ /// Create a new builder for the function named `name`
+ /// (the result of [`AggregateUDFImpl::name`])
+ pub fn new(name: &'a str, params: &'a AggregateFunctionParams) -> Self {
+ Self {
+ name,
+ params,
+ supports_within_group_clause: false,
+ }
+ }
+
+ /// Set whether the function supports the `WITHIN GROUP` clause
+ /// (the result of [`AggregateUDFImpl::supports_within_group_clause`]).
+ /// Defaults to `false`.
+ pub fn with_supports_within_group_clause(
+ mut self,
+ supports_within_group_clause: bool,
+ ) -> Self {
+ self.supports_within_group_clause = supports_within_group_clause;
+ self
+ }
+
+ /// Build the schema name
+ pub fn build(self) -> Result<String> {
+ let Self {
+ name,
+ params,
+ supports_within_group_clause,
+ } = self;
+
+ let AggregateFunctionParams {
+ args,
+ distinct,
+ filter,
+ order_by,
+ null_treatment,
+ } = params;
+
+ // exclude the first function argument(= column) in ordered set aggregate function,
Review Comment:
this logic is all the same -- there is just now some more builder ceremony logic
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org