fix(unparser): unparse a stacked aggregate as a derived table

[claudespice]

Summary (root cause)

A SELECT expresses a single grouping, but the LogicalPlan::Aggregate arm of
select_to_sql_recursively recursed straight into its input whenever the select list had
already been built (select.already_projected()), so a second aggregate stacked
underneath was skipped entirely — its GROUP BY never reached the emitted SQL. The
Sort, Limit, Distinct and Projection arms all guard the same situation by emitting
a derived table; Aggregate did not.

single_distinct_to_groupby produces exactly that shape for count(DISTINCT c): an outer
count(alias1) over an inner Aggregate grouping by c AS alias1. Unparsing the
optimized plan for SELECT count(DISTINCT "UserID") FROM hits therefore emitted

SELECT count(alias1) AS c FROM hits          -- inner GROUP BY silently gone

Two consequences, both bad:

1. alias1 does not exist on the base table, so a consumer that pushes the optimized plan
   down to a remote engine gets a binder error (Referenced column "alias1" not found in FROM clause). This is the failure reported downstream in DuckDB agg pushdown: Clickbench/aliasing issues spiceai#8475 against
   ClickBench q5 on DuckDB.
2. Where a column of that name does exist, the statement binds and the DISTINCT is
   silently gone — count(alias1) counts every row.

Changes

• SelectBuilder tracks whether an aggregate has been folded into the current SELECT
  (mark_aggregated() / already_aggregated()).
• The Aggregate arm emits a derived_aggregate table for a second aggregate instead of
  skipping it, matching the existing derived_sort / derived_limit / derived_distinct /
  derived_projection handling.

After the fix:

SELECT count(alias1) AS c FROM (SELECT hits."UserID" AS alias1 FROM hits GROUP BY hits."UserID")

Why the existing roundtrip suites missed it

datafusion/core/tests/sql/unparser.rs runs the TPC-H and ClickBench queries through
df.logical_plan() — the plan as the SQL planner produces it, before
single_distinct_to_groupby runs. The rewrite only appears in the optimized plan, which
is what a federation/pushdown consumer unparses. The new test closes that gap.

Test plan

• cargo test -p datafusion-sql — stacked_aggregate_is_unparsed_as_a_derived_table
  covers the ungrouped count(DISTINCT b) shape, the grouped
  a, count(DISTINCT b) ... GROUP BY a shape, and a lone aggregate (which must still fold
  into its SELECT rather than become a derived table).
  Result: 498 passed / 22 failed, versus 497 / 22 on a pristine checkout of this
  base — the same 22 pre-existing failures, byte-identical output, plus the new test.
• cargo test -p datafusion --test core_integration sql:: —
  test_optimized_plan_roundtrip_count_distinct unparses the optimized plan for
  count(DISTINCT) with and without a GROUP BY, then executes both the original and the
  unparsed SQL against the ClickBench fixture and compares the rows.
  Result: 71 passed / 29 failed, versus 70 / 30 on the same base — the only delta
  is the new test flipping from failing to passing.
• Neutering check: with plan.rs reverted, both new tests fail, and the core test fails
  with column 'alias1' not found and the emitted SQL
  SELECT count(alias1) AS c FROM hits_raw AS hits — i.e. it reproduces the reported bug.

Downstream issue: spiceai/spiceai#8475

---

Follow-up: the derived table's enclosing SELECT kept qualifiers that bind to nothing

Copilot's review found a defect in the shape above, confirmed on both threads. Once the
inner aggregate becomes a derived table, the enclosing SELECT reads from that and no
longer from the relation its expressions still name, so those references bind to nothing.
Only the projection and ORDER BY were swept for such a qualifier, and only at the top
level. Measuring each clause through this path:

shape	before
GROUP BY test.a	GROUP BY test.a — dangling
GROUP BY test.a + 1	dangling in both the projection and the GROUP BY, since the sweep only matches a top-level SelectItem::UnnamedExpr(CompoundIdentifier)
HAVING test.a > 1	HAVING (test.a > 1) — dangling
query-level ORDER BY	already de-qualified — the control

DataFusion re-plans such SQL, but PostgreSQL answers 42703 and DuckDB a Binder Error, so a
federated pushdown fails at the remote engine — the same class of failure as the original
bug, on the SQL the fix for it emits.

Fix

The derived table now carries an alias for every dialect, and a reference is rewritten onto
that alias only when its qualifier names a relation the derived table encloses:

SELECT derived_aggregate.a, COUNT(alias1) AS "count(DISTINCT test.b)"
FROM (SELECT test.a, test.b AS alias1 FROM test GROUP BY test.a, test.b) AS derived_aggregate
GROUP BY derived_aggregate.a

De-qualifying to a bare column name was the first attempt and is wrong under a join.
LogicalPlan::Join passes the same SelectBuilder into the left-side recursion and records
the join on it only after that recursion returns, so at the point the derived table is
built nothing on the builder shows that the SELECT reads from two relations. With a column
name shared by both sides it produced

SELECT COUNT(a), a FROM (SELECT test.a FROM test GROUP BY test.a) INNER JOIN "other" ON (test.a = "other".a) GROUP BY a

— count(test.a) reduced to COUNT(a) and GROUP BY other.a to GROUP BY a, which is
ambiguous and groups by a different column than the plan. Worse than the unbindable
qualifier it replaced, hence the alias plus the qualifier test.

The clauses are reached through one SelectBuilder::visit_expressions_in_clauses_mut rather
than new per-clause getters, so the builder's fields stay private.

Deliberate limitation

An expression holding a subquery is skipped whole. A correlated subquery names an enclosing
query's relation with a qualifier indistinguishable by name — in
HAVING test.a > (SELECT … WHERE other.c = test.a) both are test.a — so rewriting inside
one repointed the correlated reference at the derived table and silently changed which
column it read. Such an expression therefore keeps its own dangling qualifier, which is the
pre-existing output: a loud bind error at the remote engine rather than wrong rows.

Also unchanged: a join's ON is attached after this SELECT's clauses and so is not swept —
filed as spiceai/spiceai#12695, with the join test pinning the current output so the gap
stays visible.

Two derived aggregates on one join

A join walks both of its sides with one SelectBuilder, so both sides can carry a stacked
aggregate and derive a table into the same FROM clause. Under a fixed alias both were
named derived_aggregate, which is a duplicate table name most engines reject — and,
underneath that, each side requalifies its own references onto its own alias, so the two
sides' distinct columns collapsed onto one qualifier. Where the column name is shared that
binds successfully and groups by whichever side was walked first, i.e. wrong rows rather
than a loud error.

The alias is therefore numbered per SELECT (derived_aggregate_1, derived_aggregate_2,
…), following the _unnest_N precedent already on SelectBuilder — the right scope
precisely because it is the object shared across a join.

Test plan

• cargo test -p datafusion-sql — 506 passed / 22 failed. The failing set and its
  per-snapshot content are byte-identical to a pristine spiceai-54, whose 22 failures are
  pre-existing.
• cargo test -p datafusion --test core_integration sql::unparser — 3 passed, including
  the TPC-H and ClickBench roundtrips and test_optimized_plan_roundtrip_count_distinct.
• cargo clippy -p datafusion-sql --all-targets — no new warnings.
• Five new tests: every clause requalified (including a nested qualifier and HAVING), a
  dialect that requires a derived-table alias, the join case, the correlated subquery, and
  stacked aggregates on both join sides — that last one uses a column name shared by both
  sides deliberately, so a collapsed qualifier still binds and the test has to catch the
  silent-wrong-side case rather than only a bind error.
• Neuter matrix 8/8 — skipping requalification, dropping GROUP BY/HAVING from the
  swept clauses, matching only top-level expressions, dropping the qualifier guard, using a
  bare name instead of the alias, leaving the derived table unaliased, and (control)
  removing the subquery guard are each caught by the test written for them.
• For the alias numbering, a further 3/3: freezing the counter, fixing the table alias
  while leaving the requalification target numbered, and fixing the requalification target
  while leaving the table alias numbered are all caught. Because the alias appears in every
  snapshot those neuters fail the whole set, so the load-bearing check is separate and
  direct: the new both-sides test was run against the pre-fix commit and fails there,
  emitting the duplicate alias and the collapsed qualifier.

[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 #191 (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

Fixes the SQL unparser so stacked LogicalPlan::Aggregate nodes are emitted as a derived table rather than accidentally skipping the inner aggregate’s GROUP BY (notably for the optimizer shape produced by single_distinct_to_groupby for COUNT(DISTINCT ...)), and adds regression coverage for both plan-to-SQL snapshots and optimized-plan roundtrips.

Changes:

• Track whether an aggregate has already been folded into the current SELECT (SelectBuilder::aggregated), and derive the input when encountering a stacked aggregate.
• Update the unparser’s LogicalPlan::Aggregate handling to emit a derived_aggregate subquery when needed.
• Add new tests covering stacked-aggregate unparsing and optimized-plan roundtrip execution for COUNT(DISTINCT ...).

Reviewed changes

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

File	Description
datafusion/sql/src/unparser/plan.rs	Add derived-table emission for stacked aggregates during SELECT reconstruction.
datafusion/sql/src/unparser/ast.rs	Add SelectBuilder state to track whether an aggregate has been folded into the current SELECT.
datafusion/sql/tests/cases/plan_to_sql.rs	Add snapshot test coverage for stacked aggregates (including grouped and ungrouped shapes).
datafusion/core/tests/sql/unparser.rs	Add execution-based roundtrip tests for unparsing optimized plans containing COUNT(DISTINCT ...) rewrites.

---

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

datafusion/sql/src/unparser/rewrite.rs:580

• requalify_column_onto_derived_table currently treats all but the last identifier segment as the relation qualifier. This fails for nested field access like test.c1.metadata.product.name (common in this codebase), where only the leading test is the relation and the remaining segments are column/field access. In stacked-aggregate derived-table cases, such references will not be rewritten and can remain out-of-scope/dangling in the outer SELECT.

Consider rewriting based on the longest prefix of idents[..i] (for any i < len) that matches derived_qualifiers, then replace just that prefix with the derived-table alias while preserving the remaining identifier path.

    let qualifier = idents
        .iter()
        .take(idents.len() - 1)
        .map(|ident| ident.value.clone())
        .collect::<Vec<String>>()
        .join(".");
    if !derived_qualifiers.contains(&qualifier) {
        return;
    }
    let Some(last) = idents.last() else {
        unreachable!("CompoundIdentifier must have a last element");
    };
    *idents = vec![alias.clone(), last.clone()];

[Copilot]

Pull request overview

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

Suppressed comments (1)

datafusion/sql/src/unparser/rewrite.rs:581

• requalify_column_onto_derived_table assumes a CompoundIdentifier is always just <qualifier>.<column> and rewrites it to [alias, last]. However, the unparser also uses CompoundIdentifier for nested field access (e.g. get_field(col("test.a"), "b") becomes test.a.b in expr.rs:get_field_to_sql), where identifiers beyond the first can be part of the column path rather than a relation qualifier. In those cases the current logic either won’t match (qualifier becomes test.a) or would drop the a segment, leaving dangling test qualifiers or incorrect field access when a stacked aggregate becomes a derived table. Consider matching derived qualifiers against a prefix of idents and replacing only that matched prefix with the derived-table alias, preserving the remaining identifier chain (e.g. test.a.b -> derived_aggregate_1.a.b).

    let qualifier = idents
        .iter()
        .take(idents.len() - 1)
        .map(|ident| ident.value.clone())
        .collect::<Vec<String>>()
        .join(".");