neilconway commented on code in PR #20943:
URL: https://github.com/apache/datafusion/pull/20943#discussion_r2936919043
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -1748,6 +1748,32 @@ fn select_simple_aggregate_with_groupby_can_use_alias() {
);
}
+#[test]
+fn select_count_with_group_by_all() {
Review Comment:
The two tests in this file pass without the fix. Probably best to remove
them?
##########
datafusion/core/tests/sql/aggregates/basic.rs:
##########
@@ -103,6 +103,21 @@ async fn count_aggregated() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn count_aggregated_group_by_all_alias() -> Result<()> {
Review Comment:
Seems like this would be better as an SLT test?
##########
datafusion/sql/src/select.rs:
##########
@@ -192,16 +192,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
})
.collect::<Result<Vec<Expr>>>()?
} else {
- // 'group by all' groups wrt. all select expressions except
'AggregateFunction's.
- // Filter and collect non-aggregate select expressions
+ // 'group by all' groups wrt. all select expressions except
aggregate expressions.
+ // Note: aggregate expressions can be nested under one or more
aliases
+ // (e.g. count(1) AS count(*) AS c), so detect aggregates
recursively.
Review Comment:
This comment is a bit misleading, because that SQL statement doesn't parse.
The problem seems to actually occur in practice because of a (nested) alias
inserted by the analyzer, which AFAICS you can't do from SQL. I'd probably just
remove the change to the comment.
--
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]
