docs: refine AggregateUDFImpl::is_ordered_set_aggregate documentation

[Jefffrey]

Going through some tickets related to ordered set aggregates and got a little confused on DataFusion's support for them.

As I understand it, #13511 made WITHIN GROUP mandatory for ordered set aggregate functions, of which we support only two so far:

• approx_percentile_cont
  • Technically approx_median shares some internals with approx_percentile_cont but itself isn't an ordered set aggregation
• approx_percentile_cont_with_weight (which uses approx_percentile_cont internally)

This was then amended in #16999 to make it optional, at least via the SQL API; it is still mandatory on the DataFrame API:

datafusion/datafusion/functions-aggregate/src/approx_percentile_cont.rs

Lines 53 to 58 in bbb5cc7

	/// Computes the approximate percentile continuous of a set of numbers
	pub fn approx_percentile_cont(
	order_by: Sort,
	percentile: Expr,
	centroids: Option<Expr>,
	) -> Expr {

I'm updating the doc here to try clarify things to my understanding, as a followup to the original doc update: #17744

[Jefffrey]

A question I have is if we should loosen the DataFrame API to allow omitting the sort, as #16999 did for the SQL API?

cc @alamb

[Jefffrey]

Technically we don't enforce the default order; our only ordered set aggregate functions internally use ascending as the default, so I'm not sure if we should instead say its implementation dependent or try to enforce it somehow?

[Jefffrey]

I hope I'm correct in this; some reading I used for reference: https://paquier.xyz/postgresql-2/postgres-9-4-feature-highlight-within-group/

[alamb]

This was then amended in #16999 to make it optional, at least via the SQL API; it is still mandatory on the DataFrame API:

In my mind this is mostly for backwards compatibility reasons -- #13511 basically broke a bunch of our existing user queries, so I wanted to revert the unnecessarily strict interpretation

As I understand it, #13511 made WITHIN GROUP mandatory for ordered set aggregate functions, of which we support only two so far:

Indeed -- and both of these functions have the property that many times their argument will be the same as the ORDER BY WITHIN GROUP-- for example, computing approx_median(x) implicitly means approx_median(x ORDER BY x WITHIN GROUP)

Though allowing different arguments means you can write expressions like approx_median(first_name ORDER BY salary WITHIN GROUP) and save yourself a subquery

A question I have is if we should loosen the DataFrame API to allow omitting the sort, as #16999 did for the SQL API?

cc @alamb

I suggest we hold off unless someone explicitly asks about it, though I am not opposed to it either

[alamb]

Thank you @Jefffrey -- this seems like an improvement to me

[alamb]

this is a good clarification

If DataFusion ever supports more ordered set aggregation functions, we may want to revisit this

In addition to saying what this setting doesn't do, maybe we could also say what setting it to true does do? Specifically, it seems like it only affects the output display somehow 🤔

[Jefffrey]

In addition to saying what this setting doesn't do, maybe we could also say what setting it to true does do?

This is a good point 🤔

Let me look into the code a bit more to clarify my understanding and I'll update the doc accordingly.

[Jefffrey]

In my mind this is mostly for backwards compatibility reasons -- #13511 basically broke a bunch of our existing user queries, so I wanted to revert the unnecessarily strict interpretation

I suggest we hold off unless someone explicitly asks about it, though I am not opposed to it either

I might raise a separate issue to track keeping the SQL API & DataFrame API in parity in regards to this; especially for when we consider adding more ordered set aggregate functions, if we should enforce WITHIN GROUP for those but not existing ones.

Though allowing different arguments means you can write expressions like approx_median(first_name ORDER BY salary WITHIN GROUP) and save yourself a subquery

I'm a bit confused by this example; is this just a hypothetical or something that is feasible with ordered set aggregate functions? I thought they would expected one column/expression which is the same as the ORDER BY in the WITHIN GROUP 🤔

[alamb]

I'm a bit confused by this example; is this just a hypothetical or something that is feasible with ordered set aggregate functions? I thought they would expected one column/expression which is the same as the ORDER BY in the WITHIN GROUP

I am clearly a little confused myself. Now I am not sure if there is some example where the arguments differ 🤔

[Jefffrey]

I'm a bit confused by this example; is this just a hypothetical or something that is feasible with ordered set aggregate functions? I thought they would expected one column/expression which is the same as the ORDER BY in the WITHIN GROUP

I am clearly a little confused myself. Now I am not sure if there is some example where the arguments differ 🤔

I'll move this PR back to draft and do some more research to update the docs, since it seems we're both still confused about this 😅