cj-zhukov commented on code in PR #20421:
URL: https://github.com/apache/datafusion/pull/20421#discussion_r2973707951
##########
datafusion/sql/src/expr/function.rs:
##########
@@ -867,6 +905,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
Ok((exprs, names))
}
+ #[expect(dead_code)]
Review Comment:
I think I may be missing part of the intended scope, so I’d appreciate some
clarification.
It looks like we may need broader test coverage here - not only for
`quantile_cont`, but also for other aggregate functions and similar usage
patterns, to better define the expected behavior.
Originally, this issue was about rejecting invalid `ORDER BY` usage inside
aggregate argument lists earlier. My initial fix enforced that, but it
introduced a breaking change, so I updated the implementation to support both
syntaxes for `quantile_cont`. This works for the cases I’ve added tests for.
Regarding your example:
```sql
SELECT approx_percentile_cont_with_weight(1, 0.95 ORDER BY x)
FROM (VALUES (1), (2), (3)) AS t(x);
```
I didn’t consider this kind of usage across other aggregate functions when
implementing the change, so I’m not fully sure what the expected behavior
should be here.
Would you expect inline `ORDER BY` to be supported only for `quantile_cont`
(and similar ordered-set aggregates), or should this be extended more generally
to other aggregate functions as well?
--
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]
