You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Phoenix500526 (via GitHub)" <gi...@apache.org> on 2026/07/05 13:30:05 UTC
[PR] fix: preserve aggregate scope when unparsing [datafusion]
Phoenix500526 opened a new pull request, #23327:
URL: https://github.com/apache/datafusion/pull/23327
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #23317.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
The unparser can fold a Projection into its Aggregate and then keep walking into the Aggregate input. When that input is another Aggregate or an unnamed derived Projection, the generated SQL moved references above the SELECT block that defined them. Optimizer aliases such as `group_alias_0` and base-table qualifiers such as `c.signup_date` were no longer in scope.
Stop crossing those scope boundaries blindly. Render nested aggregate inputs as derived relations, and when an aggregate reads from a derived Projection, rewrite only input-schema columns to the derived output names before rendering SELECT and GROUP BY expressions.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
No
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve aggregate scope when unparsing [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve aggregate scope when unparsing [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #23327:
URL: https://github.com/apache/datafusion/pull/23327
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve aggregate scope when unparsing [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23327:
URL: https://github.com/apache/datafusion/pull/23327#issuecomment-4995375614
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23327?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.44444%` with `4 lines` in your changes missing coverage. Please review.
:warning: Please [upload](https://docs.codecov.com/docs/codecov-uploader) report for BASE (`main@95de385`). [Learn more](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#section-missing-base-commit) about missing BASE report.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23327?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/sql/src/unparser/plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/23327?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Funparser%2Fplan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3VucGFyc2VyL3BsYW4ucnM=) | 94.44% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23327?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23327 +/- ##
=======================================
Coverage ? 80.65%
=======================================
Files ? 1086
Lines ? 366165
Branches ? 366165
=======================================
Hits ? 295334
Misses ? 53238
Partials ? 17593
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23327?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve aggregate scope when unparsing [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #23327:
URL: https://github.com/apache/datafusion/pull/23327#issuecomment-4995362848
I took a quick look and it looks reasonable to me -- than k you @Phoenix500526 and @kosiew
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve aggregate scope when unparsing [datafusion]
Posted by "Phoenix500526 (via GitHub)" <gi...@apache.org>.
Phoenix500526 commented on code in PR #23327:
URL: https://github.com/apache/datafusion/pull/23327#discussion_r3549751496
##########
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:
Good catch. The sql has already parsed in `assert_issue_23317_unparsed_sql_plans` now.
##########
datafusion/sql/src/unparser/plan.rs:
##########
Review Comment:
Done in commit b56a339
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org