jayzhan211 commented on code in PR #26484:
URL: https://github.com/apache/datafusion/pull/26484#discussion_r4314507140
##########
datafusion/functions-aggregate/src/correlation.rs:
##########
@@ -152,9 +155,8 @@ impl AggregateUDFImpl for Correlation {
}
fn distinct_handling(&self) -> DistinctHandling {
- // Duplicate-sensitive, but the accumulator does not read
- // `is_distinct` and today silently returns the non-distinct answer.
- // The tag records the intent; enforcement is a follow-up change.
+ // The accumulator rejects `DISTINCT` with `not_impl_err!`, so the
+ // planner has to deduplicate the input first.
Review Comment:
nit: the planner only removes `DISTINCT` here when `SingleDistinctToGroupBy`
applies, which needs a single distinct field like `corr(DISTINCT x, x)`. Every
other case now fails. Maybe word it like this (same for the copies in
`covariance.rs` and `regr.rs`):
```diff
- // The accumulator rejects `DISTINCT` with `not_impl_err!`, so the
- // planner has to deduplicate the input first.
+ // The accumulator rejects `DISTINCT` with `not_impl_err!`. It only
+ // works when the planner removes it first
(`SingleDistinctToGroupBy`
+ // for a single distinct field, e.g. `corr(DISTINCT x, x)`).
```
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -432,6 +432,23 @@ GROUP BY g
3 NULL
4 NULL
+# With two different arguments there is no single field to group by, so
+# DISTINCT reaches the accumulator, which does not implement it.
+statement error DataFusion error: This feature is not implemented:
CORR\(DISTINCT\) aggregations are not available
+SELECT corr(DISTINCT x, y) FROM (VALUES (1.0, 1.0), (1.0, 1.0), (2.0, 3.0))
t(x, y)
+
+statement error DataFusion error: This feature is not implemented:
CORR\(DISTINCT\) aggregations are not available
+SELECT g, corr(DISTINCT x, y) FROM (VALUES (1, 1.0, 1.0), (1, 1.0, 1.0), (1,
2.0, 3.0)) t(g, x, y) GROUP BY g
+
+statement error DataFusion error: This feature is not implemented:
COVAR_SAMP\(DISTINCT\) aggregations are not available
+SELECT covar_samp(DISTINCT x, y) FROM (VALUES (1.0, 1.0), (1.0, 1.0), (2.0,
3.0)) t(x, y)
+
+statement error DataFusion error: This feature is not implemented:
COVAR_POP\(DISTINCT\) aggregations are not available
+SELECT covar_pop(x, y), covar_pop(DISTINCT x, y) FROM (VALUES (1.0, 1.0),
(1.0, 1.0), (2.0, 3.0)) t(x, y)
+
+statement error DataFusion error: This feature is not implemented:
REGR_COUNT\(DISTINCT\) aggregations are not available
+SELECT regr_count(x, y), regr_count(DISTINCT x, y) FROM (VALUES (1.0, 1.0),
(1.0, 1.0), (2.0, 3.0)) t(x, y)
Review Comment:
nit: could you add a window case too? `f(DISTINCT ..) OVER (...)` reaches
the check through `create_sliding_accumulator` rather than `accumulator`, so a
test keeps that path covered if one of these functions ever overrides it. I
checked locally that both of these already fail with the new error:
```sql
statement error DataFusion error: This feature is not implemented:
CORR\(DISTINCT\) aggregations are not available
SELECT corr(DISTINCT x, y) OVER () FROM (VALUES (1.0, 1.0), (1.0, 1.0),
(2.0, 3.0)) t(x, y)
statement error DataFusion error: This feature is not implemented:
REGR_SLOPE\(DISTINCT\) aggregations are not available
SELECT regr_slope(DISTINCT x, y) OVER () FROM (VALUES (1.0, 1.0), (1.0,
1.0), (2.0, 3.0)) t(x, y)
```
--
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]
