Skip to content

feat: change approx percentile/median UDFs to return floats - #21074

Merged
Jefffrey merged 28 commits into
apache:mainfrom
theirix:rm-aggregates-integers
Apr 18, 2026
Merged

feat: change approx percentile/median UDFs to return floats#21074
Jefffrey merged 28 commits into
apache:mainfrom
theirix:rm-aggregates-integers

Conversation

@theirix

@theirixtheirix commented Mar 20, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

  1. Migrating to the modern TypeSignature API: 264030c/datafusion/expr-common/src/signature.rs

  2. Coercing types of approx_percentile_cont, approx_percentile_cont_with_weight, approx_median to floats. It matches PostgreSQL, DuckDB, and ClickHouse behaviour, except for Spark.

What changes are included in this PR?

  • Port remaining UDFs (approx_percentile_cont, approx_percentile_cont_with_weight, approx_median, stub functions) to signature APIs
  • Deprecate INTEGERS and NUMERICS arrays in favour of using the TypeSignature API
  • They are not removed yet, but marked as deprecated to avoid breaking downstream
  • Fix up a SLT for approx_percentile_cont, approx_median to make sure it returns a float

Are these changes tested?

  • Tests are passing
  • Updated tests to expect floats in return types

Are there any user-facing changes?

  • Signatures of approx_percentile_cont, approx_percentile_cont_with_weight, approx_median changed, so they now return floats instead of integers (as seen in tests)

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Mar 20, 2026
@github-actionsgithub-actionsBot added the core Core DataFusion crate label Mar 20, 2026
@github-actionsgithub-actionsBot added the optimizer Optimizer rules label Mar 20, 2026
@theirix
theirix marked this pull request as ready for review March 20, 2026 11:31
@theirixtheirix mentioned this pull request Mar 20, 2026
Closed
Jefffrey
Jefffrey reviewed Mar 22, 2026
View reviewed changes

@JefffreyJefffrey 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 taking this up. Some of my concerns which held me off on this effort was the impact on error messaging (though I recall there was a recent PR to clean this up #20605) and if this deals with null type inputs correctly now (see #19458)

Also it seems some of the return types are changing now, I think we should call this out?

Comment threaddatafusion/expr-common/src/type_coercion/aggregates.rs
Comment threaddatafusion/sqllogictest/test_files/aggregate.slt
Comment threaddatafusion/expr-common/src/signature.rs
@theirix

theirix commented Mar 23, 2026

Copy link
Copy Markdown
ContributorAuthor

Thanks for taking this up. Some of my concerns which held me off on this effort was the impact on error messaging (though I recall there was a recent PR to clean this up #20605) and if this deals with null type inputs correctly now (see #19458)

Also it seems some of the return types are changing now, I think we should call this out?

Thank you for the review!

Added more tests for null - seems like it's consistent. The return value of these UDFs was float before (it is seen in UDF doc and also in other SQL engines), so it's reasonable to expect floats in tests too. Do you mean this change of types?

@theirix
theirix requested a review from JefffreyMarch 31, 2026 18:20
Jefffrey
Jefffrey reviewed Apr 16, 2026
View reviewed changes

@JefffreyJefffrey 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.

Added more tests for null - seems like it's consistent. The return value of these UDFs was float before (it is seen in UDF doc and also in other SQL engines), so it's reasonable to expect floats in tests too. Do you mean this change of types?

Yes, just referring to how it seems we used to return the input type but now we'll always coerce to float. We can see the return type changing in the SLTs, so would be good to highlight this in the title + PR body (since now we're going beyond a simple refactor)

Comment threaddatafusion/functions-aggregate/src/approx_percentile_cont.rs
signature: Signature::one_of(variants, Volatility::Immutable),
}
// Additionally accept an integer number of centroids for T-Digest
let signature = Signature::one_of(

@JefffreyJefffreyApr 16, 2026

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.

Since we're now coercing to floats, we can remove some of the implementation code that handles integer types

@theirixtheirixApr 16, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agree, removed some code. It also affected approx_median, piggybacking on approx_percentile_cont - updated signature and tests as well.

Clarified the scope of this PR in description - it changes more than expected

@theirixtheirix changed the title Refactor percentile UDFs to use TypeSignature APIfeat: refactor percentiles to TypeSignature, coerce to floatsApr 16, 2026
Jefffrey
Jefffrey approved these changes Apr 17, 2026
View reviewed changes

@JefffreyJefffrey 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.

Makes sense to change these to return floats since they are meant to be continuous instead of discrete 👍

@theirix

theirix commented Apr 17, 2026

Copy link
Copy Markdown
ContributorAuthor

Yes, it makes sense. Thank you for the review!

@JefffreyJefffrey changed the title feat: refactor percentiles to TypeSignature, coerce to floatsfeat: change approx percentile/median UDFs to return floatsApr 17, 2026
@Jefffrey

Jefffrey commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

With the output type changing, I wonder if that is a significant enough change to warrant an entry in the upgrade guide 🤔

@github-actionsgithub-actionsBot added the documentation Improvements or additions to documentation label Apr 17, 2026
@theirix

theirix commented Apr 17, 2026

Copy link
Copy Markdown
ContributorAuthor

With the output type changing, I wonder if that is a significant enough change to warrant an entry in the upgrade guide 🤔

I agree. Added to the PR description and to the 54.0.0 upgrade docs. We can also mention it in #21080

@JefffreyJefffrey added the api change Changes the API exposed to users of the crate label Apr 18, 2026
@Jefffrey
Jefffrey added this pull request to the merge queueApr 18, 2026
@Jefffrey

Jefffrey commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Thanks @theirix

Merged via the queue into apache:main with commit 29f1acdApr 18, 2026
36 checks passed
Rich-T-kid pushed a commit to Rich-T-kid/datafusion that referenced this pull request Apr 21, 2026
@theirix@Rich-T-kid
feat: change approx percentile/median UDFs to return floats (apache#2…
09deafb
…1074)
## Which issue does this PR close?
- Closesapache#18092.
## Rationale for this change
1. Migrating to the modern TypeSignature API:
[264030c/datafusion/expr-common/src/signature.rs](https://github.com/apache/datafusion/blob/264030cca76d0bdb4d8809f252b422e72624a345/datafusion/expr-common/src/signature.rs)
2. Coercing types of `approx_percentile_cont`,
`approx_percentile_cont_with_weight`, `approx_median` to floats. It
matches PostgreSQL, DuckDB, and ClickHouse behaviour, except for Spark.
## What changes are included in this PR?
- Port remaining UDFs (approx_percentile_cont,
approx_percentile_cont_with_weight, approx_median, stub functions) to
signature APIs
- Deprecate INTEGERS and NUMERICS arrays in favour of using the
TypeSignature API
- They are not removed yet, but marked as deprecated to avoid breaking
downstream
- Fix up a SLT for approx_percentile_cont, approx_median to make sure it
returns a float
## Are these changes tested?
- Tests are passing
- Updated tests to expect floats in return types
## Are there any user-facing changes?
- Signatures of `approx_percentile_cont`,
`approx_percentile_cont_with_weight`, `approx_median` changed, so they
now return floats instead of integers (as seen in tests)
@theirixtheirix mentioned this pull request May 6, 2026
Closed
55 tasks
@coderabbitaicoderabbitaiBot mentioned this pull request Aug 12, 2026
Merged
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JefffreyJefffreyJefffrey approved these changes

Assignees

No one assigned

Labels

api changeChanges the API exposed to users of the cratecoreCore DataFusion cratedocumentationImprovements or additions to documentationfunctionsChanges to functions implementationlogical-exprLogical plan and expressionsoptimizerOptimizer rulessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor away usage of NUMERICS/INTEGERS in datafusion/expr-common/src/type_coercion/aggregates.rs

2 participants

@theirix@Jefffrey