Inline ORDER BY drops arguments for approximate percentile aggregates

[haohuaijin]

Describe the bug

Inline ORDER BY handling unconditionally removes the first aggregate argument.

This breaks approx_percentile_cont, where the first argument is the percentile rather than the ordered value.

To Reproduce

CREATE TABLE aggregate_test_repro AS
SELECT c1, c3
FROM (
    VALUES
        ('a', CAST(10 AS DOUBLE)),
        ('a', CAST(20 AS DOUBLE)),
        ('b', CAST(30 AS DOUBLE))
) AS t(c1, c3);

SELECT c1, approx_percentile_cont(0.95, 200 ORDER BY c3)
FROM aggregate_test_repro
GROUP BY c1;

The query fails with:

Percentile value must be between 0.0 and 1.0 inclusive, 200 is invalid

The weighted variant also loses its weight argument:

approx_percentile_cont_with_weight(1, 0.95 ORDER BY c3)

Expected behavior

No response

Additional context

No response

[haohuaijin]

take

[jayzhan211]

SELECT c1, approx_percentile_cont(0.95, 200 ORDER BY c3)
FROM aggregate_test_repro
GROUP BY c1;

Is this valid query in DataFusion?

https://duckdb.org/docs/lts/sql/functions/aggregates#ordered-set-aggregate-functions

Don't we need WITHIN GROUP for ordered set aggregation function like DuckDB

[jayzhan211]

Upd: I found this #16955 🤔, could we support ordering for WITHIN GROUP syntax only and not support it for old syntax @alamb

#18281

I think we should support WITHIN GROUP for these four functions rather than fixing the issue for incorrect usage. What do you think, @haohuaijin?

                // Built-in ordered-set aggregates must also support WITHIN GROUP
                let is_builtin_ordered_set = matches!(
                    name.as_str(),
                    "percentile_cont"
                        | "quantile_cont"
                        | "approx_percentile_cont"
                        | "approx_percentile_cont_with_weight"
                );

[haohuaijin]

Upd: I found this #16955 🤔, could we support ordering for WITHIN GROUP syntax only and not support it for old syntax @alamb

i remember is we both support old syntax and new syntax(because i remember when i upgrade datafusion, i only can write new syntax(WHTHIN GROUP), and then next release both support old syntax and new syntax), the duckdb suport both WITHIN GROUP and order by inline

https://duckdb.org/docs/lts/sql/functions/aggregates#ordered-set-aggregate-functions

[Jefffrey]

i think i was aiming for not supporting order by for ordered set aggregates, per

• Disallow order by within ordered-set aggregate functions argument lists #18281

however it seems the PR that closes it pivoted to allowing it to avoid breaking queries, though confusingly the title and body doesnt reflect this pivot

• Disallow order by within ordered-set aggregate functions argument lists #20421

at this point since we allow it i guess we'd just have to keep supporting it