Unparser drops ORDER BY alias when flattening Projection through SubqueryAlias#21491
Merged
Jefffrey merged 12 commits intoapache:mainfrom Apr 23, 2026
Merged
Unparser drops ORDER BY alias when flattening Projection through SubqueryAlias#21491Jefffrey merged 12 commits intoapache:mainfrom
Jefffrey merged 12 commits intoapache:mainfrom
Conversation
…gregate When the SQL unparser encountered a SubqueryAlias node whose direct child was an Aggregate (or other clause-building plan like Window, Sort, Limit, Union), it would flatten the subquery into a simple table alias, losing the aggregate entirely. For example, a plan representing: SELECT j1.col FROM j1 JOIN (SELECT max(id) AS m FROM j2) AS b ON j1.id = b.m would unparse to: SELECT j1.col FROM j1 INNER JOIN j2 AS b ON j1.id = b.m dropping the MAX aggregate and the subquery. Root cause: the SubqueryAlias handler in select_to_sql_recursively would call subquery_alias_inner_query_and_columns (which only unwraps Projection children) and unparse_table_scan_pushdown (which only handles TableScan/SubqueryAlias/Projection). When both returned nothing useful for an Aggregate child, the code recursed directly into the Aggregate, merging its GROUP BY into the outer SELECT instead of wrapping it in a derived subquery. The fix adds an early check: if the SubqueryAlias's direct child is a plan type that builds its own SELECT clauses (Aggregate, Window, Sort, Limit, Union), emit it as a derived subquery via self.derive() with the alias always attached, rather than falling through to the recursive path that would flatten it.
…gregate When the SQL unparser encountered a SubqueryAlias node whose direct child was an Aggregate (or other clause-building plan like Window, Sort, Limit, Union), it would flatten the subquery into a simple table alias, losing the aggregate entirely. For example, a plan representing: SELECT j1.col FROM j1 JOIN (SELECT max(id) AS m FROM j2) AS b ON j1.id = b.m would unparse to: SELECT j1.col FROM j1 INNER JOIN j2 AS b ON j1.id = b.m dropping the MAX aggregate and the subquery. Root cause: the SubqueryAlias handler in select_to_sql_recursively would call subquery_alias_inner_query_and_columns (which only unwraps Projection children) and unparse_table_scan_pushdown (which only handles TableScan/SubqueryAlias/Projection). When both returned nothing useful for an Aggregate child, the code recursed directly into the Aggregate, merging its GROUP BY into the outer SELECT instead of wrapping it in a derived subquery. The fix adds an early check: if the SubqueryAlias's direct child is a plan type that builds its own SELECT clauses (Aggregate, Window, Sort, Limit, Union), emit it as a derived subquery via self.derive() with the alias always attached, rather than falling through to the recursive path that would flatten it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rojection In `rewrite_plan_for_sort_on_non_projected_fields`, when the outer Projection excludes a Sort column that was defined as an alias in the inner Projection (e.g. `Z AS c`), the rewrite replaced the inner Projection's expressions with only the outer Projection's mapped expressions, dropping the alias definition. This left ORDER BY referencing a non-existent column. The fix inlines the underlying physical expression into the Sort when an alias is dropped (e.g. `ORDER BY c` → `ORDER BY t."Z"`). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clarify what the algorithm does and why, remove example-specific column names from the implementation comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Jefffrey
approved these changes
Apr 21, 2026
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
I think once CI is green this should be good to go
Contributor
Author
Thanks @Jefffrey . Would appreciate a merge. |
Contributor
|
Thanks @yonatan-sevenai |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
When
plan_to_sqlencounters aProjection → Sort → Projection → SubqueryAliasplan where the outer Projection excludes a Sortcolumn defined as an alias in the inner Projection (e.g.
Z AS c), the functionrewrite_plan_for_sort_on_non_projected_fieldsflattens the two Projections into one but only keeps the outer Projection's columns. This drops the alias definition, leaving
ORDER BY creferencing a column that no longer exists in the generated SQL.What changes are included in this PR?
In
rewrite_plan_for_sort_on_non_projected_fields(datafusion/sql/src/unparser/rewrite.rs), when the inner Projection istrimmed to only the outer Projection's expressions, sort expressions that reference dropped aliases are now inlined to the
underlying physical expression. For example,
ORDER BY cbecomesORDER BY t."Z"whencwas defined asZ AS cin thedropped inner Projection.
Are these changes tested?
Yes. A new regression test
test_sort_on_aliased_column_dropped_by_outer_projectionindatafusion/sql/tests/cases/plan_to_sql.rsconstructs the exact plan shape that triggers the bug and asserts the correct SQLoutput:
All existing
plan_to_sqltests (118 tests) continue to pass.Are there any user-facing changes?
No API changes. Previously invalid SQL is now generated correctly.