Skip to content

Keep aggregate udaf schema names unique when missing an order-by #17731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 25, 2025
Merged

Keep aggregate udaf schema names unique when missing an order-by #17731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cbeb1c
test: reproducer of bug
wiedld Sep 18, 2025
b4c633f
fix: make schema names unique for approx_percentile_cont
wiedld Sep 22, 2025
a01f8f6
test: regression test is now resolved
wiedld Sep 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion datafusion/expr/src/udaf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ pub fn udaf_default_schema_name<F: AggregateUDFImpl + ?Sized>(

// exclude the first function argument(= column) in ordered set aggregate function,
// because it is duplicated with the WITHIN GROUP clause in schema name.
let args = if func.is_ordered_set_aggregate() {
let args = if func.is_ordered_set_aggregate() && !order_by.is_empty() {
Copy link
Contributor

@alamb alamb Sep 23, 2025

Choose a reason for hiding this comment

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

This clearly fixes a bug so I think we can merge this. However, it seems somewhat strange to me given that my understanding of ordered set aggregate functions is that they require an ORDER BY clause 🤔

I wonder if this PR is just fixing a symptom, and the root cause is that approx_percentile_cont should actually be created with an order by expression 🤔

I tried to update the docs here

Copy link
Contributor Author

@wiedld wiedld Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this is because we are currently supporting the corrected syntax (which includes an order-by), as well as the older syntax which does not include an order-by in the UDAF clause itself.

Docs: https://datafusion.apache.org/user-guide/sql/aggregate_functions.html#approx-percentile-cont

New, corrected syntax:
SELECT approx_percentile_cont(0.75) WITHIN GROUP (ORDER BY column_name) FROM table_name;

Older syntax, still supported:
SELECT approx_percentile_cont(column_name, 0.75) FROM table_name;

Copy link
Contributor Author

@wiedld wiedld Sep 24, 2025

Choose a reason for hiding this comment

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

Note that this bug occurred with customers using the older syntax, since it was shortening the schema names in such a way that they were non-unique is only the 1st argument was different.

&args[1..]
} else {
&args[..]
Expand Down
23 changes: 23 additions & 0 deletions datafusion/sqllogictest/test_files/aggregate.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1821,6 +1821,29 @@ c 122
d 124
e 115


# using approx_percentile_cont on 2 columns with same signature
Copy link
Contributor

@alamb alamb Sep 23, 2025

Choose a reason for hiding this comment

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

I verified this test fails as follows without the code change


1. query failed: DataFusion error: Schema error: Schema contains duplicate unqualified field name "approx_percentile_cont(Float64(0.95))"
[SQL] SELECT c1, approx_percentile_cont(c2, 0.95) AS c2, approx_percentile_cont(c3, 0.95) AS c3 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
at /Users/andrewlamb/Software/datafusion/datafusion/sqllogictest/test_files/aggregate.slt:1840

query TII
SELECT c1, approx_percentile_cont(c2, 0.95) AS c2, approx_percentile_cont(c3, 0.95) AS c3 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
----
a 5 73
b 5 68
c 5 122
d 5 124
e 5 115

# error is unique to this UDAF
query TRR
SELECT c1, avg(c2) AS c2, avg(c3) AS c3 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
----
a 2.857142857143 -18.333333333333
b 3.263157894737 -5.842105263158
c 2.666666666667 -1.333333333333
d 2.444444444444 25.444444444444
e 3 40.333333333333



query TI
SELECT c1, approx_percentile_cont(0.95) WITHIN GROUP (ORDER BY c3 DESC) AS c3_p95 FROM aggregate_test_100 GROUP BY 1 ORDER BY 1
----
Expand Down