fix(unparser): keep ORDER BY out of a derived table when the sort key is computed

[claudespice]

Summary (root cause)

rewrite_plan_for_sort_on_non_projected_fields hoists a Sort sandwiched between two Projections to the top of the plan, so the unparsed statement carries the ORDER BY itself. The gate was a set equality on expression strings: the inner Projection's outputs had to be exactly the outer Projection's expressions plus the sort keys.

A sort key that is an expression over a projected column never matched that gate. ORDER BY age + 1 contributes the string person.age + Int64(1) while the inner Projection exposes person.age, so the rewrite bailed out and select_to_sql_recursively fell into its derived_sort branch:

-- SELECT id FROM person ORDER BY age + 1 DESC   unparsed to:
SELECT id FROM (SELECT person.id, person.age FROM person ORDER BY (person.age + 1) DESC NULLS FIRST)

SQL does not require an enclosing query to preserve the ordering of a derived table. The ORDER BY is buried where the remote engine is free to ignore it, so a federated SELECT ... ORDER BY <expression> comes back in arbitrary order — silently, with no error.

ORDER BY age (a bare column) was unaffected, which is why this went unnoticed: the failure needs the sort key to be computed.

Changes

• When a sort key is not itself one of the inner Projection's outputs, account for the columns it reads instead. An inner Projection that exists only to expose those columns then matches, and the Sort is hoisted as intended. A sort key does not have to be a projected output for the hoist to be valid — it only has to be computable from them.
• Hoisting replaces the inner Projection's expressions, so any alias it defined is dropped. The existing dropped-alias substitution only fired when the whole sort expression was that alias; it now substitutes nested references too, so ORDER BY x + 1 over a dropped expr AS x becomes ORDER BY expr + 1 rather than a dangling x.

Test plan

• cargo test -p datafusion-sql — 499 passed, 22 failed. Those 22 are pre-existing on spiceai-54: the same 22 tests with byte-identical snapshot diffs fail on an unmodified checkout of the base commit (b8b95926). Verified by diffing the full run output before and after this change — the failure set and every snapshot assertion are unchanged. The delta is 497 -> 499 passing, i.e. only the two tests added here.
• cargo test -p datafusion --test tpcds_planning — 198 passed, no change.
• Swept all 99 TPC-DS queries (optimize -> plan_to_sql) checking that a plan rooted at Sort always emits a top-level ORDER BY: 0 lost before and after, and the same 4 pre-existing grouping id unparse errors (q27/q36/q70/q86). No plan's output changed.
• cargo clippy -p datafusion-sql --all-targets clean; cargo fmt clean for both touched files (expr.rs has pre-existing fmt drift, left alone).

Tests added

• order_by_over_non_projected_field_stays_top_level — the expression sort key (ORDER BY age + 1), the aggregate-expression variant (ORDER BY max(age) + 1), and the bare-column case that already worked, so the generalisation cannot regress it.
• order_by_nested_reference_to_dropped_alias — sort key referencing a dropped inner alias from inside a larger expression.

Neutered-fix check: with rewrite.rs reverted and the tests kept, both new tests fail (the emitted SQL is the derived-table form above) while the 33 existing order_by_* tests still pass. The regression guards genuinely bite.

Note on scope

The reported case (spiceai/spiceai#6931, Dremio TPC-DS q91) no longer reproduces through q91 itself on DataFusion 54 — the optimizer now leaves Sort at the plan root there. The unparser hazard behind it was still live and is what this fixes; the sweep above is the evidence that no TPC-DS query is currently affected while the one-line query SELECT id FROM person ORDER BY age + 1 is.

[claudespice]

This PR has been open and green for 5.6 days with no reviewer ever requested and no review activity. For context, recent merges in this repo landed in 15 minutes to ~2.7 days (#181–#189), so this is well outside the normal window.

I don't have write access on this fork, so I can't add a reviewer or assignee myself — flagging it here instead. Two siblings are in the same state: #190 (physical-optimizer) and #192 (unparser).

Nothing is blocking it technically: CI is green and it merges cleanly into spiceai-54. It just needs someone to take a look.

[grokspice]

@copilot review

[grokspice]

@copilot review

[Copilot]

Pull request overview

This PR fixes an unparser rewrite that could incorrectly emit ORDER BY inside a derived table when the sort key is a computed expression over non-projected columns, which can silently lose ordering because SQL does not guarantee derived-table ordering is preserved by an enclosing query.

Changes:

• Generalize rewrite_plan_for_sort_on_non_projected_fields to treat sort expressions as valid for hoisting when they are computable from (i.e., reference) the inner projection’s outputs, not only when they exactly match them.
• Expand dropped-alias rewriting so references nested inside larger sort expressions are substituted, preventing dangling alias references after hoisting.
• Add regression tests covering computed sort keys and nested references to dropped aliases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
datafusion/sql/src/unparser/rewrite.rs	Adjusts the Sort-hoisting rewrite to account for sort-key column dependencies and rewrites nested dropped-alias references in sort expressions.
datafusion/sql/tests/cases/plan_to_sql.rs	Adds regression tests ensuring top-level ORDER BY is preserved and dropped-alias references are correctly substituted.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.