kosiew commented on code in PR #23327:
URL: https://github.com/apache/datafusion/pull/23327#discussion_r3549033535
##########
datafusion/sql/src/unparser/plan.rs:
##########
Review Comment:
Nice work applying the new aggregate input normalization to SELECT and GROUP
BY.
I think the same path is needed for HAVING predicates that come from a
`Filter` over an `Aggregate`.
For the same plan shape being fixed here, where `Aggregate` reads from an
unnamed derived `Projection`, a query like `... GROUP BY 1 HAVING
count(DISTINCT cs.customer_id) > 0` still unparses to `HAVING (count(DISTINCT
\"cs\".\"customer_id\") > 0)`. The FROM item is `(SELECT
\"cs\".\"customer_id\", ... FROM ...)`, so the `cs` qualifier is no longer in
scope.
Could we apply the same `normalize_agg_input_columns` path after
`unproject_agg_exprs` here, and also in the QUALIFY branch when it unprojects
aggregate expressions? That should make all expressions rendered in the
aggregate SELECT resolve against the derived projection output.
##########
datafusion/core/tests/sql/unparser.rs:
##########
@@ -399,6 +402,118 @@ async fn
optimized_duckdb_unparse_qualifies_nested_passthrough_column() -> Resul
Ok(())
}
+// https://github.com/apache/datafusion/issues/23317
+//
+// `SingleDistinctToGroupBy` can produce two stacked Aggregates where the inner
+// Aggregate defines intermediate fields such as `group_alias_0`, `alias1`, and
+// `alias2`. The unparser must preserve that inner Aggregate as a derived table
+// before the outer Aggregate references those fields.
+//
+// Without `SingleDistinctToGroupBy`, the Aggregate still sits over an unnamed
+// derived Projection. In that SQL scope, base table aliases `cs` and `c` are
no
+// longer visible, so aggregate expressions must refer to the derived table's
+// output columns unqualified.
+const ISSUE_23317_QUERY: &str = r#"
+WITH cohort AS (
+ SELECT
+ signup_year,
+ sum(customers) AS customers,
+ sum(revenue) AS revenue
+ FROM
+ (
+ SELECT
+ date_part('year', c.signup_date) AS signup_year,
+ count(DISTINCT cs.customer_id) AS customers,
+ round(sum(cs.total_revenue), 2) AS revenue
+ FROM
+ "warehouse"."main"."sales" cs
+ JOIN "warehouse"."main"."customers" c USING (customer_id)
+ GROUP BY
Review Comment:
Small suggestion for the regression coverage: it would be great if the
`issue_23317` tests executed, or at least parsed, the unparsed SQL rather than
only checking string fragments.
The bug here is invalid SQL caused by out-of-scope aliases, so a parse or
roundtrip execution assertion would be more likely to catch similar missed
clauses like HAVING or ORDER BY even if the exact formatting changes later.
--
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]
