Make udaf_default_* display/schema name helpers non-generic (214 KB smaller binary) - #24728
Make udaf_default_* display/schema name helpers non-generic (214 KB smaller binary)#24728alamb wants to merge 3 commits into
Conversation
Sorry, something went wrong.
This comment was marked as outdated.
alamb
left a comment
There was a problem hiding this comment.
This Diff looks lage, but if you ignore whitespace via https://github.com/apache/datafusion/pull/24728/changes?w=1 it is mich smaller (basically adding a bunch of docs and a builder)
Sorry, something went wrong.
| /// 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) |
There was a problem hiding this 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
Sorry, something went wrong.
| null_treatment, | ||
| } = params; | ||
|
|
||
| // exclude the first function argument(= column) in ordered set aggregate function, |
There was a problem hiding this comment.
this logic is all the same -- there is just now some more builder ceremony logic
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| /// Builds the default implementation of [`AggregateUDFImpl::schema_name`]. |
There was a problem hiding this comment.
Here is a new builder for schema name -- it is basically the same pattern for all of the other types
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 79.39698% with 41 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #24728 +/- ##
==========================================
- Coverage 81.47% 81.47% -0.01%
==========================================
Files 1122 1122
Lines 404140 404185 +45
Branches 404140 404185 +45
==========================================
+ Hits 329284 329292 +8
- Misses 55546 55577 +31
- Partials 19310 19316 +6 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Which issue does this PR close?
Rationale for this change
The default udaf_default_* display / schema name helpers in datafusion/expr/src/udaf.rs are instantiated for each UDAF
What changes are included in this PR?
Code size
Measured per the methodology in #24727, at merge base d66f4b9 and on this branch (39d0e53).
cargo llvm-lines --release -p datafusion-functions-aggregate --lib (29 AggregateUDFImpl implementations):
cargo llvm-lines --release -p datafusion-expr --lib (the builder bodies now compile here exactly once):
cargo build --release --bin datafusion-cli:
Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
Yes, but not breaking: