Skip to content

Commit ebd12eb

Browse files
chore: tidy up comments in sort alias fix
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>
1 parent ed46d9d commit ebd12eb

2 files changed

Lines changed: 16 additions & 18 deletions

File tree

datafusion/sql/src/unparser/rewrite.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -254,12 +254,13 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
254254
.map(|e| map.get(e).unwrap_or(e).clone())
255255
.collect::<Vec<_>>();
256256

257-
// Build a reverse map from alias name → underlying expression for
258-
// sort columns that are dropped from the outer projection. When
259-
// the inner Projection is trimmed to `new_exprs` below, any alias
260-
// that only existed in the inner Projection disappears, so ORDER BY
261-
// references to it become dangling. We inline the physical
262-
// expression instead (e.g. ORDER BY "c" → ORDER BY "Z").
257+
// The inner Projection may define aliases that the Sort references
258+
// but the outer Projection does not include. Since we are about to
259+
// replace the inner Projection's expressions with `new_exprs` (which
260+
// only contains the outer Projection's columns), those alias
261+
// definitions will be lost. To keep the Sort valid, rewrite any
262+
// sort expression that references a dropped alias so that it uses
263+
// the alias's underlying expression instead.
263264
let projected_aliases: HashSet<&str> = new_exprs
264265
.iter()
265266
.filter_map(|e| match e {
@@ -268,7 +269,7 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
268269
})
269270
.collect();
270271

271-
let alias_to_underlying: HashMap<String, Expr> = inner_p
272+
let dropped_aliases: HashMap<String, Expr> = inner_p
272273
.expr
273274
.iter()
274275
.filter_map(|e| match e {
@@ -279,15 +280,14 @@ pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
279280
})
280281
.collect();
281282

282-
if !alias_to_underlying.is_empty() {
283+
if !dropped_aliases.is_empty() {
283284
for sort_expr in &mut sort.expr {
284285
let mut expr = sort_expr.expr.clone();
285286
while let Expr::Alias(alias) = expr {
286287
expr = *alias.expr;
287288
}
288289
if let Expr::Column(ref col) = expr {
289-
let name = col.name();
290-
if let Some(underlying) = alias_to_underlying.get(name) {
290+
if let Some(underlying) = dropped_aliases.get(col.name()) {
291291
sort_expr.expr = underlying.clone();
292292
}
293293
}

datafusion/sql/tests/cases/plan_to_sql.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2974,12 +2974,12 @@ fn test_unparse_manual_join_with_subquery_aggregate() -> Result<()> {
29742974
Ok(())
29752975
}
29762976

2977-
/// Regression test: when the outer Projection excludes a Sort column that was
2978-
/// defined as an alias in the inner Projection (through a SubqueryAlias), the
2979-
/// Unparser must either preserve the subquery boundary or inline the physical
2980-
/// column name into the ORDER BY clause.
2977+
/// Regression test for https://github.com/apache/datafusion/issues/21490
29812978
///
2982-
/// See: https://github.com/apache/datafusion/issues/XXXX
2979+
/// When the outer Projection excludes a Sort column whose definition only
2980+
/// exists as an alias in the inner Projection, the Unparser must inline the
2981+
/// underlying expression into ORDER BY rather than emitting the now-missing
2982+
/// alias name.
29832983
#[test]
29842984
fn test_sort_on_aliased_column_dropped_by_outer_projection() -> Result<()> {
29852985
let schema = Schema::new(vec![
@@ -3017,9 +3017,7 @@ fn test_sort_on_aliased_column_dropped_by_outer_projection() -> Result<()> {
30173017
let unparser = Unparser::default();
30183018
let sql = unparser.plan_to_sql(&plan)?;
30193019

3020-
// The sort column "c" (aliased from "Z" in the inner Projection) must be
3021-
// inlined to the physical column in ORDER BY, since the outer Projection
3022-
// excludes it from the SELECT list.
3020+
// ORDER BY must reference the physical column, not the dropped alias.
30233021
assert_snapshot!(
30243022
sql,
30253023
@r#"SELECT t."X" AS a, t."Y" AS b FROM phys_table AS t ORDER BY t."Z" DESC NULLS FIRST LIMIT 1"#

0 commit comments

Comments
 (0)