refactor(unparser): centralize aggregate-scope rendering in the SQL unparser

[naman-modi]

Which issue does this PR close?

• Closes Centralize aggregate-scope expression rendering in the SQL unparser #23668.

Rationale for this change

• When printing a GROUP BY query, the unparser sometimes wraps the aggregate's input in an inner subquery (... FROM (SELECT ...)).
• Table aliases like cs only exist inside that subquery. Any clause outside it (SELECT, GROUP BY, HAVING, QUALIFY, ORDER BY) must use the subquery's output columns, not the aliases, so the unparser drops the alias.
• Each clause does that dropping on its own, so it is easy to miss one.
• ORDER BY on an aggregate that is not in the SELECT list was missed: it printed ORDER BY round(sum("cs"."total_revenue"), 2), while the SELECT list in the same query correctly printed sum("total_revenue").
• DataFusion reads that SQL back fine, but stricter databases reject it because cs is out of scope there.

What changes are included in this PR?

I moved the rule into one helper, UnparserAggScope, that checks once per aggregate whether the input is an inner subquery and then prepares expressions for each clause. SELECT, GROUP BY, HAVING, QUALIFY, and both ORDER BY paths now go through it, which fixes the ORDER BY case. The window-over-aggregate path is left as-is with a comment: it is only reachable from hand-built plans, since a window in SQL always sits inside a SELECT that already drops the alias. Only ORDER BY output changes; every already-correct case stays the same.

Are these changes tested?

Yes. New tests in datafusion/core/tests/sql/unparser.rs cover the inner-subquery shape for a window sorting by an aggregate, ORDER BY on an unselected aggregate (the fixed case), and a top-level ORDER BY (the second sort path). Existing unparser tests and the TPC-H/Clickbench roundtrips still pass.

Are there any user-facing changes?

ORDER BY over an aggregate is now printed without an out-of-scope table qualifier, so the generated SQL is valid for stricter databases. No API changes.

[naman-modi]

Hey @kosiew, can you please review this PR, when you get a chance?

One small design note: I went with a small UnparserAggScope struct that holds the aggregate and the "is the input a derived subquery" flag, computed once and reused by every clause, rather than a bare helper fn taking the flag as a parameter.

IMO, it reads a bit cleaner at the call sites and keeps the flag from being recomputed per clause. If you'd rather keep it consistent with the existing impl Unparser helpers, I'm happy to drop the struct and move it to a plain associated fn instead.

TIA!

[kosiew]

@naman-modi
Nice fix overall. I like the direction of consolidating the aggregate scope handling into a small helper. I just have a couple of minor suggestions that could make the abstraction a little cleaner and the test coverage a bit clearer.

[kosiew]

I like UnparserAggScope as a small abstraction because it naturally ties an aggregate to its derived-input scope decision.

One small thing that stood out is prepare_sort_expr(scope: Option<&Self>, ...). The None case doesn't really have an aggregate scope, so it feels like the abstraction is carrying a responsibility outside its model.

Would it make sense to keep aggregate normalization as an instance method and move the optional sort orchestration into an Unparser helper, or simply split the aggregate and non-aggregate paths? I think that would keep the responsibilities a bit more focused. Just a small design suggestion.

[naman-modi]

Makes sense, I have updated the prepare_sort_expr to now be an instance method of UnparserAggScope, and created a Unparser::unproject_sort_expr_in_scope to handle both the arms, hence unifying the call sites.

[kosiew]

This test is useful because it exercises the top-level Sort path, but since it orders by the selected alias customers, it doesn't actually demonstrate that the new prepare_sort_expr logic strips an aggregate input qualifier in that branch.

Would it be worth adding a direct top-level Sort regression whose sort expression contains an aggregate over a qualified input column? Alternatively, the test name or comment could be adjusted to make it clear that it's only covering the top-level Sort path. The non-selected aggregate regression already covers the reported bug.

[naman-modi]

You're right, this holds. I have updated the method name & comment to be more specific - that it only covers the top-level Sort routing.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 82.92683% with 7 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@05b7e11). Learn more about missing BASE report.
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/sql/src/unparser/plan.rs	82.92%	0 Missing and 7 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #23789   +/-   ##
=======================================
  Coverage        ?   80.71%           
=======================================
  Files           ?     1089           
  Lines           ?   368903           
  Branches        ?   368903           
=======================================
  Hits            ?   297764           
  Misses          ?    53375           
  Partials        ?    17764           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[naman-modi]

Thanks for reviewing @kosiew! I've addressed both review comments. It would be great, if you could trigger the CI.