Phoenix500526 commented on code in PR #27159: URL: https://github.com/apache/datafusion/pull/27159#discussion_r4510523945
########## datafusion/sqllogictest/test_files/aggregate.slt: ########## @@ -759,6 +759,14 @@ SELECT approx_median(c12) FROM aggregate_test_100 statement error SELECT approx_median(c1) FROM aggregate_test_100 +# APPROX_MEDIAN ignores aggregate ORDER BY keys. They must not be interpreted Review Comment: Thanks @alamb for pointing this out. My initial approach came from the discussion in [#9924](https://github.com/apache/datafusion/issues/9924#issuecomment-2037406494) about removing ORDER BY when it does not affect the aggregate’s result. I wanted to stay consistent with functions such as AVG and MEDIAN, which were marked Insensitive in [#25711](https://github.com/apache/datafusion/pull/25711). I checked how other systems handle aggregate-local ORDER BY: - PostgreSQL accepts it for ordinary aggregates, including AVG and COUNT, and honors the specified input order. See [Aggregate Expressions](https://www.postgresql.org/docs/18/sql-expressions.html#SYNTAX-AGGREGATES). - DuckDB accepts it and discards it for aggregates classified as order-insensitive, while preserving it for order-sensitive aggregates. See [ORDER BY in Aggregate Functions](https://duckdb.org/docs/2.0/sql/functions/aggregates#order-by-clause-in-aggregate-functions). - MySQL 8.4 does not allow this syntax for AVG or COUNT, but explicitly supports it for GROUP_CONCAT. See [Aggregate Function Descriptions](https://dev.mysql.com/doc/refman/8.4/en/aggregate-functions.html). - SQLite accepts aggregate-local ORDER BY, including for functions such as COUNT, whose result does not depend on ordering. See [Built-in Aggregate Functions](https://www.sqlite.org/lang_aggfunc.html). DuckDB provides a closer comparison through approx_quantile(x, 0.5), which also uses t-digest to estimate the median. See [Approximate Aggregates](https://duckdb.org/docs/2.0/sql/functions/aggregates#approximate-aggregates). It accepts and honors: ```SQL ❯ duckdb DuckDB v2.0.0 (Cyanoptera) Enter ".help" for usage hints. memory D SET threads = 1; memory D CREATE TABLE median_order_test AS SELECT (i * i)::DOUBLE AS x, i AS y FROM range(1, 100001) t(i); memory D SELECT approx_quantile(x, 0.5) FROM median_order_test; ┌─────────────────────────┐ │ approx_quantile(x, 0.5) │ │ double │ ├─────────────────────────┤ │ 2500197917.5 │ │ (2.50 billion) │ └─────────────────────────┘ memory D SELECT approx_quantile(x, 0.5 ORDER BY y ASC) FROM median_order_test; ┌────────────────────────────────────────┐ │ approx_quantile(x, 0.5 ORDER BY y ASC) │ │ double │ ├────────────────────────────────────────┤ │ 2500197917.5 │ │ (2.50 billion) │ └────────────────────────────────────────┘ memory D SELECT approx_quantile(x, 0.5 ORDER BY y DESC) FROM median_order_test; ┌─────────────────────────────────────────┐ │ approx_quantile(x, 0.5 ORDER BY y DESC) │ │ double │ ├─────────────────────────────────────────┤ │ 2500634840.5 │ │ (2.50 billion) │ └─────────────────────────────────────────┘ memory D DROP TABLE median_order_test; ``` Changing the input order changes the estimate. I also reproduced order-dependent estimates in DataFusion’s accumulator when reordering values across batches, so my original justification for Insensitive was too broad. I propose following DuckDB: preserve the existing ordering behavior and fix the shared accumulator so it does not interpret the sort key as a percentile argument. APPROX_MEDIAN would keep the percentile fixed at 0.5 and process x in the requested order. Does this approach make sense to you? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
