Enforce explicit opt-in for `WITHIN GROUP` syntax in aggregate UDAFs #18607

kosiew · 2025-11-11T08:33:58Z

Which issue does this PR close?

Rationale for this change

Previously, the SQL planner accepted WITHIN GROUP clauses for all aggregate UDAFs, even those that did not explicitly support ordered-set semantics. This behavior was too permissive and inconsistent with PostgreSQL. For example, queries such as SUM(x) WITHIN GROUP (ORDER BY x) were allowed, even though SUM is not an ordered-set aggregate.

This PR enforces stricter validation so that only UDAFs that explicitly return true from supports_within_group_clause() may use WITHIN GROUP. All other aggregates now produce a clear planner error when this syntax is used.

What changes are included in this PR?

Added type alias WithinGroupExtraction to simplify complex tuple return types used by helper functions.
Introduced a new helper method extract_and_prepend_within_group_args to centralize logic for handling WITHIN GROUP argument rewriting.
Updated the planner to:
- Validate that only UDAFs with supports_within_group_clause() can accept WITHIN GROUP.
- Prepend WITHIN GROUP ordering expressions to function arguments only for supported ordered-set aggregates.
- Produce clear error messages when WITHIN GROUP is used incorrectly.
Added comprehensive unit tests verifying correct behavior and failure cases:
- WITHIN GROUP rejected for non-ordered-set aggregates (MIN, SUM, etc.).
- WITHIN GROUP accepted for ordered-set aggregates such as percentile_cont.
- Validation for named arguments, multiple ordering expressions, and semantic conflicts with OVER clauses.
Updated SQL logic tests (aggregate.slt) to reflect new rejection behavior.
Updated documentation:
- aggregate_functions.md and developer docs to clarify when and how WITHIN GROUP can be used.
- upgrading.md to inform users of this stricter enforcement and migration guidance.

Are these changes tested?

✅ Yes.

New tests in sql_integration.rs validate acceptance, rejection, and argument behavior of WITHIN GROUP for both valid and invalid cases.
SQL logic tests (aggregate.slt) include negative test cases confirming planner rejections.

Are there any user-facing changes?

✅ Yes.

Users attempting to use WITHIN GROUP with regular aggregates (e.g. SUM, AVG, MIN, MAX) will now see a planner error:

WITHIN GROUP is only supported for ordered-set aggregate functions
Documentation has been updated to clearly describe WITHIN GROUP semantics and provide examples of valid and invalid usage.

No API-breaking changes were introduced; only stricter planner validation and improved error messaging.

…s in planner

…gates

…ive aggregates

Ensure SQL WITHIN GROUP(...) is only allowed for UDAFs that opt into ordered-set semantics. Cached the support check locally and updated the condition to return an error when the clause is incorrectly used. Added a unit test to verify this rejection for non-ordered-set aggregate functions.

…UDAFs

…UDAFs and enhance upgrade documentation for stricter planner enforcement

…DAFs and remove obsolete test file

…n for non-ordered-set aggregates

Refactor within_group_rejected_for_non_ordered_set_udaf to extract the planner error using .expect_err(...).to_string() to avoid brittle Debug formatting issues. Add within_group_allowed_for_ordered_set_udaf to test percentile_cont_udaf() registration with the planner, ensuring the planning of WITHIN GROUP invocation succeeds and verifying that the resulting plan mentions percentile_cont.

… WITHIN GROUP clause

…ntile_cont UDAF

…_integration tests

Add a concise comment for explicit opt-in in the planner. Extract argument-prepending logic into a helper method to centralize the protocol. Add tests to validate rejection of multiple ORDER BY items inside WITHIN GROUP and the combination of USING WITHIN GROUP with OVER.

…rn types and improve readability

Jefffrey

Can I ask how much of this PR was LLM generated? I see a lot of hallmarks of LLM generated code.

Jefffrey · 2025-11-11T15:11:37Z

datafusion/sql/src/expr/function.rs

+// Type alias to simplify very complex tuple return types used by helper
+// functions in this module. Using a named alias reduces the type complexity
+// shown in compiler errors and improves readability.


These comments feel overly verbose

Also having this type alias so far away from its only usage feels odd to me

Removed the comment.
Moving the alias closer to fn extract_and_prepend_within_group_args generated this compiler error
error[E0658]: inherent associated types are unstable

On a stable Rust release, this errors is resolved by moving the type alias outside of the impl block and making it a top-level type alias

Jefffrey · 2025-11-11T15:11:47Z

datafusion/sql/src/expr/function.rs

+                // Enforce explicit opt-in for WITHIN GROUP: UDAFs must advertise
+                // `supports_within_group_clause()` to accept WITHIN GROUP syntax.
+                // Previously the planner forwarded WITHIN GROUP to any
+                // order-sensitive aggregate; that was too permissive.
+                let supports_within_group = fm.supports_within_group_clause();
+
+                if !within_group.is_empty() && !supports_within_group {
+                    return plan_err!(
+                        "WITHIN GROUP is only supported for ordered-set aggregate functions"
+                    );
+                }
+
+                // If the UDAF opted into WITHIN GROUP handling, convert the
+                // WITHIN GROUP ordering into sort expressions and prepend the
+                // ordering expressions to the function argument list (as unnamed
+                // args). This helper centralizes the argument-prepending protocol
+                // so it's easier to maintain and reason about.


Ditto for these comments

Amended to be more succinct.

Jefffrey · 2025-11-11T15:14:19Z

datafusion/sql/src/expr/function.rs

+        args: Vec<Expr>,
+        arg_names: Vec<Option<String>>,
+        schema: &DFSchema,
+        planner_context: &mut PlannerContext,
+    ) -> Result<WithinGroupExtraction> {
+        let mut args = args;
+        let mut arg_names = arg_names;


Suggested change

args: Vec<Expr>,

arg_names: Vec<Option<String>>,

schema: &DFSchema,

planner_context: &mut PlannerContext,

) -> Result<WithinGroupExtraction> {

let mut args = args;

let mut arg_names = arg_names;

mut args: Vec<Expr>,

mut arg_names: Vec<Option<String>>,

schema: &DFSchema,

planner_context: &mut PlannerContext,

) -> Result<WithinGroupExtraction> {

Jefffrey · 2025-11-11T15:16:29Z

datafusion/sql/tests/sql_integration.rs

I'm not sure adding these tests here are necessary; SLTs should be sufficient and I believe some of the tests added here are already in SLTs, e.g.

datafusion/datafusion/sqllogictest/test_files/aggregate.slt

Lines 174 to 179 in d57e215

# Not supported over sliding windows

query error DataFusion error: Error during planning: OVER and WITHIN GROUP clause cannot be used together. OVER is for window functions, whereas WITHIN GROUP is for ordered set aggregate functions

SELECT approx_percentile_cont(0.5)

WITHIN GROUP (ORDER BY c3)

OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)

FROM aggregate_test_100

Removed the tests that are already covered in slt.

Jefffrey · 2025-11-11T15:18:07Z

docs/source/user-guide/sql/aggregate_functions.md

+## WITHIN GROUP / Ordered-set aggregates
+
+Some aggregate functions support the SQL `WITHIN GROUP (ORDER BY ...)` clause. This clause is used by
+ordered-set aggregate functions (for example, percentile and rank-like aggregations) to specify the ordering
+of inputs that the aggregate relies on. In DataFusion, only aggregate functions that explicitly opt into
+ordered-set semantics via their implementation will accept `WITHIN GROUP`; attempting to use `WITHIN GROUP`
+with a regular aggregate (for example `SUM(x) WITHIN GROUP (ORDER BY x)`) will fail during planning with an
+error. This matches Postgres semantics and ensures ordered-set behavior is opt-in for user-defined aggregates.
+
+Example (ordered-set aggregate):
+
+```sql
+percentile_cont(0.5) WITHIN GROUP (ORDER BY value)
+```
+
+Example (invalid usage — planner will error):
+
+```sql
+-- This will fail: SUM is not an ordered-set aggregate
+SELECT SUM(x) WITHIN GROUP (ORDER BY x) FROM t;
+```


If we decide to add this to our documentation we should be explicit about which functions are ordered set aggregates. As it is, the user cannot tell by this alone.

ordered-set aggregate functions (for example, percentile and rank-like aggregations) to specify the ordering
of inputs that the aggregate relies on. In DataFusion, only aggregate functions that explicitly opt into
ordered-set semantics via their implementation will accept WITHIN GROUP; attempting to use WITHIN GROUP

I thought this part gave a good indication of ordered set aggregate functions.

kosiew · 2025-11-12T10:32:47Z

Can I ask how much of this PR was LLM generated? I see a lot of hallmarks of LLM generated code.

Thank you for your patient review and feedback.

I use VS Code with Copilot Pro, but I always review, adapt, and test everything before opening a PR.
The final code reflects my design and review decisions.

…nderstanding

…oup_args function

…ITHIN GROUP / percentile_cont)

…raction alias to module scope

kosiew added 15 commits November 11, 2025 15:26

feat: enforce WITHIN GROUP support for ordered-set aggregate function…

68cf7ae

…s in planner

feat: add documentation for WITHIN GROUP support in ordered-set aggre…

915e3bb

…gates

feat: enforce restrictions on WITHIN GROUP support for order-insensit…

1679849

…ive aggregates

feat: add test case for rejection of WITHIN GROUP in non-ordered-set …

5c33aa0

…UDAFs

feat: update test case for WITHIN GROUP rejection in non-ordered-set …

fcdce76

…UDAFs and enhance upgrade documentation for stricter planner enforcement

feat: add rejection test for WITHIN GROUP clause in non-ordered-set U…

5eaaddc

…DAFs and remove obsolete test file

feat: add error handling for WITHIN GROUP clause in array_agg functio…

9e71211

…n for non-ordered-set aggregates

feat: update test case for percentile_cont UDAF to use c3 ordering in…

62cc368

… WITHIN GROUP clause

feat: add test for named argument behavior with WITHIN GROUP in perce…

a2968ab

…ntile_cont UDAF

refactor: clean up imports and remove redundant use statements in sql…

494142f

…_integration tests

refactor: introduce type alias WithinGroupExtraction to simplify retu…

f79715a

…rn types and improve readability

clippy fix

193c6fc

github-actions bot added documentation Improvements or additions to documentation sql SQL Planner development-process Related to development process of DataFusion sqllogictest SQL Logic Tests (.slt) labels Nov 11, 2025

Jefffrey reviewed Nov 11, 2025

View reviewed changes

kosiew added 7 commits November 12, 2025 18:41

Move WithinGroupExtraction type alias closer to fn

a58a27f

refactor: clarify comments on UDAF WITHIN GROUP handling for better u…

2c2ff39

…nderstanding

refactor: simplify argument handling in extract_and_prepend_within_gr…

752c78c

…oup_args function

tests(sqllogictest): remove unit tests duplicated by aggregate.slt (W…

333a3b8

…ITHIN GROUP / percentile_cont)

sql: avoid unstable inherent associated type by moving WithinGroupExt…

1265c4b

…raction alias to module scope

Remove unused imports

566508b

cargo fmt

c5dd922

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enforce explicit opt-in for `WITHIN GROUP` syntax in aggregate UDAFs #18607

Enforce explicit opt-in for `WITHIN GROUP` syntax in aggregate UDAFs #18607

kosiew commented Nov 11, 2025

Uh oh!

Jefffrey left a comment

Uh oh!

Jefffrey Nov 11, 2025

Uh oh!

kosiew Nov 12, 2025 •

edited

Loading

Uh oh!

kosiew Nov 12, 2025

Uh oh!

Jefffrey Nov 11, 2025

Uh oh!

kosiew Nov 12, 2025

Uh oh!

Jefffrey Nov 11, 2025

Uh oh!

Jefffrey Nov 11, 2025

Uh oh!

kosiew Nov 12, 2025

Uh oh!

Jefffrey Nov 11, 2025

Uh oh!

kosiew Nov 12, 2025

Uh oh!

kosiew commented Nov 12, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# Not supported over sliding windows
	query error DataFusion error: Error during planning: OVER and WITHIN GROUP clause cannot be used together. OVER is for window functions, whereas WITHIN GROUP is for ordered set aggregate functions
	SELECT approx_percentile_cont(0.5)
	WITHIN GROUP (ORDER BY c3)
	OVER (ROWS BETWEEN 4 PRECEDING AND CURRENT ROW)
	FROM aggregate_test_100

Enforce explicit opt-in for WITHIN GROUP syntax in aggregate UDAFs #18607

Are you sure you want to change the base?

Enforce explicit opt-in for WITHIN GROUP syntax in aggregate UDAFs #18607

Conversation

kosiew commented Nov 11, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Enforce explicit opt-in for `WITHIN GROUP` syntax in aggregate UDAFs #18607

Enforce explicit opt-in for `WITHIN GROUP` syntax in aggregate UDAFs #18607

kosiew Nov 12, 2025 •

edited

Loading

kosiew commented Nov 12, 2025 •

edited

Loading