fix(unparser): keep the ORDER BY and row bound a node above already set, instead of letting a nested sort or limit overwrite them (refs spiceai/spiceai#14733)

[grokspice]

Which issue does this PR close?

Refs spiceai/spiceai#14733.

Rationale for this change

The unparser folds every Sort and Limit of a plan into one QueryBuilder, which holds a single ORDER BY and a single row bound. A node unparsed after another had already set them overwrote them, so the emitted SQL carried the inner node's keys or bound under the outer node's position — the last writer won. The issue's shapes, run in DuckDB 1.4.4 over u(a, b) = (1,9) (2,8) (3,7) (4,6) (5,5) and t(a) = 1..10, before this change (spiceai-55 at cf084aa85):

plan	emitted SQL before	rows before	plan's rows
Sort[u.a, fetch 2] over Sort[u.b]	SELECT * FROM u ORDER BY u.b ASC NULLS FIRST LIMIT 2	(5,5) (4,6)	(1,9) (2,8)
Sort[t.a, fetch 3] over Join(t, Sort[u.a desc] over u)	… ORDER BY u.a DESC NULLS LAST LIMIT 3	(5,5) (4,4) (3,3)	(1,1) (2,2) (3,3)
Limit(1) over Limit(2)	SELECT * FROM u LIMIT 2	2 rows	1 row
Sort[u.a, fetch 1] over Limit(2)	… ORDER BY u.a ASC NULLS FIRST LIMIT 2	2 rows	1 row
Projection[u.a] → Sort[(u.a + 1)] → Projection[u.a, u.b] → Sort[u.b] → Projection[u.a, u.b]	SELECT a FROM (SELECT u.a, u.b FROM u ORDER BY u.b ASC NULLS FIRST)	5 4 3 2 1	1 2 3 4 5
Sort[m] → Projection[m] → Sort[m desc] → Aggregate[max(u.a) AS m]	… ORDER BY max(u.a) DESC NULLS LAST	5 4 3 2 1	1 2 3 4 5
Sort[(t.k + 1) desc, fetch 1] → Projection[s, t.k] → Limit(2) → Aggregate[t.k; sum(t.v) AS s]	… GROUP BY t.k ORDER BY (t.k + 1) DESC NULLS FIRST LIMIT 2	2 rows	1 row

What changes are included in this PR?

The query builder now says what it already carries, and each node folds in only where it would have folded under a node the walk absorbed itself:

• QueryBuilder::has_order_by, literal_row_bound, merge_inner_row_bound (ast.rs): an inner literal bound merges with the one above through combine_limit, the way the optimizer merges stacked limits; an expression bound cannot merge.

• Limit arm: under an existing ORDER BY the limit takes a scope of its own (derive_row_limited_scope, which now walks through a Sort when the enclosing query re-establishes the order); under an existing bound it merges, or takes a scope when either bound is an expression.

• Sort arm: under an existing ORDER BY an unfetched sort is dropped (the outer order reorders it) and a fetched one takes a scope; a fetch under an existing bound merges. Inside a join input a sort's ORDER BY is dropped (a join leaves its output order unspecified) and a fetched sort becomes a derived table under the scan's name, as a join input's Limit does. A sort's keys resolve against the node under the unfetched sorts the walk drops and under a DISTINCT.

• Projection arm: the Sort/Limit fold starts from the state the query already holds, and the grouped-predicate refusal in row_limit_needs_own_scope runs before that state stops the fold. A chain the fold leaves in a derived table is derived directly with every output named (name_scope_outputs), the clauses above it re-pointed at those outputs by bare name (t.k → k; t.payload.x → payload.x once the hidden schema confirms payload is a column of the hidden t), and, when the enclosing query bounds rows without ordering them, re-ordered by the chain's own sort keys (refused when the projection computes an output named like one of those keys, which would capture it). Two outputs that share a name under the dialect's identifier folding, a clause above holding a subquery, and an outer reference in the projection named like a derived output are refused. Re-establishing the chain's order is refused in two more cases: a sort key that cannot be evaluated a second time (random()), and an outer reference in a key that a computed alias would capture. A bare ORDER BY key is refused when a computed alias whose name folds to the key's name would capture it. For a sort above the projection, the alias the key itself reads, spelled the same, is exempt. For a sort absorbed from below the projection there is no exemption, because that key reads the derived table, not the alias.

• rewrite_plan_for_sort_on_non_projected_fields: the fold has always hoisted a sort through an inner projection when the outer projection and the sort read exactly that projection's outputs (equal sets). It may now also drop inner outputs that nothing above reads (a strict subset), so the outer order stays at the top level. It does this only within a subset that is safe by construction (widened_fold_is_safe):

  • every output it keeps is a plain column named by that column, in both the inner and the outer schema;
  • the outer list reads each kept output once;
  • no inner output is volatile or holds a subquery.

  Every other shape keeps the sort in its own scope and emits exactly what spiceai-55 emits. This replaces the per-shape guards of earlier rounds; the escalation comment on this PR explains why.

  Both paths now rebuild the inner projection's schema in the replacement order (fix(unparser): scope a sort key reading a projection output that cannot be repeated (refs spiceai/spiceai#14109) #247 carries the same hunk for its own case). Both also:

  • substitute each dropped output into the key bottom-up;
  • drop a key that the substitution turns into a literal;
  • decline the fold when a key reads a dropped output that cannot be repeated, when the outer list repeats such an output, or when a kept alias would capture a substituted column or outer reference.

  On both paths the fold is also declined when two inner outputs share a display string, the spelling it matches outputs by (t."a.b" and an alias "t.a.b" both display as t.a.b). It is declined as well when an alias or plain column carries a name other than the one its schema declares (outputs_named_as_declared).

  The capture checks compare names through Unparser::emitted_column_key, so they are case-folded. A derived projection now counts as a scope that strips qualifiers, as an aggregate over one already did.

• Refusals where no faithful scope exists: a fetched sort under an expression bound, a limit under a sort over an aggregate, and a HAVING above a limited aggregate under a sort.

Shapes with one Sort/Limit, or stacked ones that already merged, emit the same SQL as before: the datafusion-sql suite's 22 pre-existing failures (Snowflake flatten, roundtrip_statement*, test_struct_expr, …) are unchanged in name and count. One existing snapshot changes: test_unparse_aggregate_with_non_literal_limit_over_sort_with_fetch gains an outer ORDER BY __agg_0 ASC NULLS FIRST before its LIMIT CAST(7 AS INTEGER), so the outer bound keeps the rows the inner sort ranked first.

Are these changes tested?

70 new tests in plan_to_sql.rs pin each shape above and every shape the reviews raised. Every round's tests were run red against the head before that round, as quoted in the commit messages and review threads. For the narrowing round, the final test file fails 13 tests beyond the pre-existing 22 when run against the ed3515dc5 sources: the 9 regression tests for this round's threads and adversarial findings, and 4 tests whose fold is now given up (listed in the PR conversation).

A mutation pass on the 7a0e80723 sources disabled each of eight fold and refusal guards on its own and re-ran the suite. Every one of them has a test that fails without it. The two refusals added after that commit are pinned by tests that fail on the commit before each one.

cargo test -p datafusion-sql --test sql_integration, compared by test name:

• this branch at 724148148, which merges spiceai-55 ddd5c8ce6: 898 passed, 22 failed;
• spiceai-55 at ddd5c8ce6: 828 passed, 22 failed;
• the 22 failures are the same set on both (test_struct_expr, snowflake_flatten_*, roundtrip_statement*, …).

cargo test -p datafusion-sql --lib (1.97.0): 103 passed. cargo clippy -p datafusion-sql --all-targets -- -D warnings: clean. cargo fmt --all -- --check: clean.

The differential check from earlier rounds:

Differential check against DuckDB 1.4.4 (Python duckdb): a probe binary unparses each shape with plan_to_sql, and the oracle runs the emitted SQL and a hand-written statement for the same plan over the same tables, comparing column count and rows (ordered where the plan orders). Before, on cf084aa85:

FAIL S1: columns=2 rows=[(5, 5), (4, 6)]   expected rows=[(1, 9), (2, 8)]
FAIL S2: columns=2 rows=[(5, 5), (4, 4), (3, 3)]   expected rows=[(1, 1), (2, 2), (3, 3)]
FAIL S3: rows=[(1, 9), (2, 8)]   expected rows=[(1, 9)]
FAIL S4: rows=[(1, 9), (2, 8)]   expected rows=[(1, 9)]
FAIL S5: rows=[(5,), (4,), (3,), (2,), (1,)]   expected rows=[(1,), (2,), (3,), (4,), (5,)]
FAIL S6: rows=[(3, 0.86…), (1, 0.23…)] (expected the two smallest u.a, ascending)
FAIL S7: rows=[(4, 6), (5, 5)]   expected rows=[(2, 8)]
FAIL S10: rows=[(5,), (4,), (3,), (2,), (1,)]   expected rows=[(1,), (2,), (3,), (4,), (5,)]
FAIL S11e: duckdb error: Binder Error: Referenced table "u" not found!
FAIL S12: rows=[(20, 2), (15, 1)]   expected rows=[(20, 2)]
RESULT: FAIL (6/17 shapes agree with DuckDB 1.4.4)

After, on this branch at d294df539, over the 33 shapes accumulated through the reviews (the issue's, the derived-aggregate forms, the fold's pruned/reordered/unnamed/volatile/shadowed/literal keys, the sort-over-sort and sort-over-DISTINCT stacks over a derived aggregate, the struct-field key, the case-folded names, the expression bound over a sorted chain):

RESULT: PASS (33/33 shapes agree with DuckDB 1.4.4)

(S6 reads random(), so the oracle checks its two rows are the two smallest u.a, ascending. A further shape, a fetched sort whose key reads a dropped random() output twice, is pinned by …dropped_volatile_output_keeps_the_sort_in_its_scope as emitting exactly what the base emits — the sort stays in its own scope; the repetition of random() inside that scope's key is spiceai/spiceai#14109's, fixed by #247 — and is kept out of the row comparison because that inner key is nondeterministic on both. The refused shapes count as agreeing: the oracle reports them as refusals, not rows.)

Are there any user-facing changes?

A federated query whose plan stacks a sort or limit under another now returns the rows and order the plan asks for from the remote engine, or is refused and answered locally, instead of returning the rows the inner node selects. A plan whose aggregate takes a derived table of its own no longer references a relation that derived table hides, names the outputs it reads through it, and keeps its order under an outer parameterised LIMIT.

Review gate

• Adversarial review: skipped — codex exited 1 on the final head 724148148, and on bdd8d629b and 05cd0b2c7, with "Your workspace is out of credits". Under grokspice, codex is the only allowed engine. The two earlier heads of this narrowing round were reviewed:

  • codex job review-mvbma1d1-uijeaw on 7a0e80723 returned needs-attention with 1 finding. A bare key from a sort absorbed below the projection is captured by (0 - s) AS s: SQLite and DuckDB return -20 where the plan returns -10. Reproduced, and also present on spiceai-55. Fixed in 56442fc82.
  • codex job review-mvbmrj3w-21cd7u on 56442fc82 returned needs-attention with 1 finding. An outer reference in a re-established key is captured by a computed alias: DuckDB returns -3 where the plan returns -1. Reproduced and fixed in 05cd0b2c7.
  • 05cd0b2c7 was read only by a read-only correctness sub-agent, which found no defects. It left two notes, both unverified, code inspection only: a qualified outer reference is over-refused (costs a pushdown, never wrong rows), and an outer reference nested inside a compound absorbed key is not checked against computed aliases.

• Copilot's two threads on bdd8d629b (display-string collision; alias declared under another name) were reproduced red on bdd8d629b and fixed in 724148148 by declining the fold. One fold an existing test expected is given up and listed on its thread. /security-review was not re-run for 724148148, which adds two early returns and no emission.

• /security-review: no findings. It was re-scoped to this worktree's diff, because the harness's default diff was a different checkout. The commit adds no new identifier or literal emission, the new indexing is bounded, the refusals are static strings, and errors make the fold fail closed.

• /simplify (read-only sub-agent): 3 behavior-neutral proposals, applied in 05cd0b2c7. One emitted_column_name call per alias, HashMap imported, and ? inside exists in place of a hand-written error fallback. No guard is unreachable on both fold paths, so none was removed.

• Pre-existing emissions found while verifying (spiceai-55 emits them too, and this PR does not change them): unparser: three wrong-result emissions spiceai-55 already has for sort shapes #263 declines #267.

• Earlier rounds: codex — four reviews ran, each returning needs-attention with findings that reproduced and are fixed here: review-mvakq4tb-dcjg8v on 40bca8df3 (an unnamed computed output dropped from the fold left ORDER BY "u.a + u.b"; a fetched sort over an unfetched sort over a derived aggregate expanded s to sum(v) outside the derived table — fixed in 5da169eb4), review-mval06t5-jupko2 on 5da169eb4 (a dropped random() output substituted at both references of the key — the fold is declined in 246bf0d35), review-mvalb534-g0yh8n on 246bf0d35 ((s + 1) AS s substituted without end, a stack overflow; a projected lit(1) substituted into ORDER BY 1, a column position — fixed in 4d1df092a), review-mvalis5g-s03gop on 4d1df092a (a surviving 0 - s AS s capturing the substituted s of a dropped s AS x — the fold is declined in a25f720bd). On the heads after a25f720bd the engine was skipped — codex exited 1: "Your workspace is out of credits. Ask your workspace owner to refill in order to continue." (receipt head=d294df539…, attempt=codex, engine=none). An earlier partial run on 543fb7445 had left two hypotheses: a stale schema after pruning (then not reproduced, later reproduced by Copilot's reordered-alias shape and fixed) and the out-of-scope qualifier above a derived aggregate (reproduced as S12, fixed in 651373395).

• Copilot's reviews (13 threads over four passes): twelve addressed — a HAVING left outside the derived chain, duplicate bare names after qualifier removal, an unaliased aggregate output at the derived boundary, the stale projection schema after a reordering fold (40bca8df3); an expression bound over a sorted chain now re-ordered outside, case-folded duplicate names, a struct-field key, an outer-reference capture, and a DISTINCT between sort and projection (43256a482, c075660ab); a second-pass substitution collision that a25f720bd already declines; and the field-path re-pointing narrowed in 0cda516fd so that the shared helper keeps its full-qualifier form (a sibling t.s.a under full qualification is untouched) and only the derived-chain scope drops a shorter prefix, with the hidden schema confirming the column; and a computed projection alias named like a re-established key ((0 - s) AS s above ORDER BY s), refused in d294df539. One thread stays open as a hypothesis: a projection alias named like the group column its key reads cannot be built (DataFusion rejects the schema as ambiguous), so no plan reaches the described capture. Note: 43256a482 was pushed with a test that did not compile (get_field unimported) because the gate read an empty failure list as clean; c075660ab fixes it, and the gate now stops when the suite prints no result line.

• Fix pass b890188b6 (spiceemma, scoped to that commit only): Copilot's 07:23Z/07:32Z pass. Reproduced first, each as a new regression test that fails on d294df539: a scalar output named like the hidden relation kept ORDER BY (t.k + 1) outside the derived table; and (0 - s) AS "S" and an outer S got past the computed-alias and outer-reference capture checks. All three are fixed (both checks now compare through emitted_column_key, and the field-path root is kept only for a struct or map output, dictionary-encoded or not). The rewrite.rs case-folded surviving-alias shape did not reproduce: the fold is already declined. It is pinned by a test, and rewrite.rs is unchanged. Adversarial review: codex, job review-mvar4vo7-m7s0ok, scoped --base HEAD~1, returned needs-attention with one finding: dictionary structs and maps also take field access. The root check was widened to cover them. Its claim that this changes the emitted SQL did not reproduce: a dictionary-struct and a map probe emitted the same SQL before and after the widening. /security-review: the diff only adds refusals and name comparisons. It adds no input surface, quoting or I/O, so there are no findings. Local gate: cargo fmt --check passes. cargo clippy -p datafusion-sql --all-targets -D warnings (1.97.1) is clean. plan_to_sql gives 445 passed / 22 failed, and the 22 are the same set that fails on base spiceai-55 (409 passed / 22 failed). Unparser lib tests: 52 passed.

• Found while probing, reproduced on d294df539 and not fixed here: a struct output t.payload AS t beside a grouping column t.x, with a fetched sort on get_field(t, 'x') above Limit → Projection[t, s], emits ... GROUP BY t.x, t.payload LIMIT 2) ORDER BY x DESC NULLS FIRST LIMIT 1. That sorts by the grouping column x, not by payload.x.

• Residual, unverified: an outer reference in a clause above the projection (a filter's) is converted to SQL before this scope is built, so the capture check in the projection does not see it.

• /security-review (read-only sub-agent over the diff): no quoting, panic or overflow findings (combine_limit saturates; LIMIT literals parse through usize::from_str). Two low, unverified, code inspection only, both pre-existing shapes this diff does not touch: peel_to_unnest_with_modifiers sets ORDER BY/LIMIT unconditionally on unnest_as_table_factor/lateral_flatten dialects (the same last-writer-wins class, for a Sort → Projection[unnest] → Sort → Unnest plan on BigQuery/Snowflake), and a fetched sort under an outer sort over a projected input derives under DERIVED_SORT_ALIAS, so an ORDER BY u.a emitted above names a qualifier the derived table hides (a remote binder error, not wrong rows).

• /simplify (read-only sub-agent): 5 proposals, all applied — has_row_limit folded into the identical bounds_rows, the Limit arm's literal-merge match folded into one if let chain, bounded computed once, and two rustdoc/comment corrections; suite, clippy and the oracle re-run green afterwards.

• Fix pass 169f91ceb + 8837afb9c (spiceemma, scoped to 9832ce2a2..8837afb9c): Copilot's three threads on 9832ce2. Each was reproduced first as a regression test that fails on 9832ce2, and checked against DuckDB. (1) Under full qualification, a key t.s.a (column of t.s) beside table t's column s was repointed to s.a, which DuckDB rejects. The full qualifier is now matched first, and a reference that is also a struct field path is refused. (2) An outer reference in an ancestor, absorbed or re-established sort key rendered as a bare s that the derived output s captured: DuckDB picked k = 1 where the plan wants k = 2. The Sort arm now records the outer references its keys read, and the capture check covers all three paths. (3) The sort fold's shadow guard ignored names declared through Projection::try_new_with_schema, so it emitted ORDER BY (a + b) (3 instead of 11). The guard now reads the folded projection's own schema. Adversarial review: codex exec --sandbox read-only on 9832ce2a2..169f91ceb returned four findings. Two reproduced and are fixed in 8837afb9c: an outer reference in a re-established order (DuckDB 2 vs 1), and the guard's names disagreeing with the schema for a repeated output (DuckDB 3 vs 11). One did not reproduce: the t / t.s / t.s.a field-path shape is already refused, and a test pins it. One is rejected as an over-refusal only: a qualified outer reference is refused like an unqualified one, because the repoint walk strips a qualifier that names a hidden relation (unverified, code inspection only). Local gate: plan_to_sql gives 456 passed / 22 failed, the same 22 by name as on 9832ce2 (448/22) and base. cargo fmt --check passes, and clippy 1.97.1 -D warnings is clean.

• Fix pass 3d8e76946 (spiceemma, scoped to 8837afb9c..3d8e76946): Copilot's three threads on 8837afb. Each was reproduced first as a regression test that fails on 8837afb9c, and the first two were checked in SQLite. (1) When the fold's field names collided, it fell back to the outer schema, so the key y + 1 resolved by position to the second copy of t.a + 1. SQLite returned 2|2|101 where the plan wants 6|6|1. The fold is now declined. (2) An unqualified outer reference in a dropped output was substituted into the key, where a kept alias of the same name captured it. SQLite returned -5 where the plan wants -1. The shadow guard now declines this case. (3) Under full qualification, a flattened get_field_path(t.s, 'a', 'b') took the longest relation prefix t.s and emitted a.b, a field access on a scalar. A prefix is now taken only where the column's type has the rest of the path, which gives s.a.b. Adversarial review: codex adversarial-review --base 8837afb9c (receipt rc=0) returned needs-attention with one finding: get_field accepts a NULL base, so the path check should accept Null. That arm is added. Unverified: the described plan could not be built, because get_field on a NULL column unparses as a bare b, on base spiceai-55 as well. /security-review could not run (the session's working directory is not a git repository). A manual read of the diff found no input surface, I/O or new panics: the slice idents[prefix_len + 1..] is in bounds because prefix_len < idents.len(), and the outer-reference walk declines the fold on error. /simplify was not run separately, since the diff is three small guards. Local gate: cargo fmt passes, and clippy 1.97.1 --all-targets -D warnings is clean. sql_integration gives 865 passed / 22 failed, and the 22 are the same set by name as on 8837afb9c (which also fails the 3 new tests) and on base.

• Fix pass 67bd2dc43 (spiceemma, scoped to 3d8e76946..67bd2dc43): Copilot's three threads on 3d8e769. Each was reproduced first as a regression test that fails on 3d8e76946. (1) Past a map, the path check accepted any remaining parts, so get_field_path(t.s, 'a', 'b', 'c') beside t.s.a: map<string,int> emitted a.b.c, a field of an int. A map part is now one key, and the rest of the path is checked against the value type, which gives s.a.b.c. (2) Two relation prefixes both read t.s.a.b as a valid path, and the longer one was stripped, giving a.b (t.s's output). This is now refused as ambiguous with the existing NotImplemented error. (3) A repeat declared under the name of a dropped inner output ([x, x] declared [z, y]) kept that output from being substituted, so the key y + 1 emitted ORDER BY ((t.a + 1) + 1). The fold now declines when a key reads such a borrowed name. Adversarial review: codex adversarial-review --base 3d8e76946 (receipt exit=0, verdict=needs-attention) returned one high finding: a declined fold leaves the ORDER BY in a derived table with nothing outside it. A probe with fetch = 3 showed that this shape predates the guard. Every decline reason produces it (the volatile-output and field-name-collision declines emit the same thing), and base emits it too, so it is tracked in unparser: a declined sort fold leaves the ORDER BY inside a derived table with nothing outside it #265 rather than fixed here. Its narrower point was valid: the guard fired even when the key did not read the borrowed name. The guard is narrowed, and …_still_folds_a_key_not_reading_it pins it. /security-review could not run (the session's working directory is not a git repository). A manual read of the diff found no input surface or I/O, and no new panics: the map arm uses .get(1). /simplify was not run separately, since the diff is three small guards. Local gate: cargo fmt --check passes, and clippy 1.97.1 --all-targets -D warnings is clean. plan_to_sql gives 463 passed / 22 failed, the same 22 by name as before the fix and on base. Unparser lib tests: 103 passed.

• Fix pass ed3515dc5 (spiceemma, scoped to 67bd2dc43..ed3515dc5): Copilot's two threads on 67bd2dc. Both were reproduced first as regression tests that fail on 67bd2dc43.

  • (1) Declared-name collision. kept_names counted an unnamed kept expression's spelled name, so a dropped output declared under that spelling was never substituted: ORDER BY "t.a + t.b". kept_names now holds only explicit aliases and declared fields, which gives ORDER BY (t.c + 1).
  • (2) Qualified substitution under a derived aggregate. A qualified substitution in an aggregate scope over a derived projection lost its qualifier, so the bare ORDER BY k bound to the kept (0 - t.k) AS k. PostgreSQL 17 returned -2 where the plan wants -1. The fold is now declined there, and returns -1.
  • Adversarial review, first pass. codex job review-mvb63ll4-xbzuur on the first version (needs-attention) found that declining every qualified read also declined joins, whose derived-table fallback emits SELECT k, v, v and loses u.v. This reproduced. The guard now applies only where contains_projection_before_relation strips qualifiers, and test_a_qualified_substitution_over_a_join_still_folds pins the fold.
  • Adversarial review, second pass. codex job review-mvb6ayqa-mfrjco (needs-attention) found that a compound key x + 1 was also declined, and the fallback's ORDER BY (x + 1) fails on PostgreSQL 17 (column "x" does not exist). This reproduced, while the fold's ORDER BY (k + 1) returns the correct -1. The guard is now limited to bare keys, and a test pins the compound fold. Its other finding (a declined fold's derived ORDER BY with a multi-row fetch) is the pre-existing shape tracked in unparser: a declined sort fold leaves the ORDER BY inside a derived table with nothing outside it #265.
  • Unverified: engines that resolve SELECT aliases inside ORDER BY expressions (MySQL) may bind k + 1 to the alias. That behavior is unchanged from 67bd2dc43.
  • /security-review could not run (the session's working directory is not a git repository). A manual read found no input surface, I/O or new panics; contains_projection_before_relation only became pub(super).
  • Local gate: cargo fmt passes, and clippy 1.97.1 --all-targets -D warnings is clean. plan_to_sql gives 467 passed / 22 failed, the same 22 by name as on 67bd2dc43 (463/22). Unparser lib tests: 103 passed.
    Overlap: fix(unparser): keep a union input's own ORDER BY and LIMIT inside that input #261 (union inputs) has merged into spiceai-55 and is merged here (bdd8d629b); the only conflict was both sides appending tests. fix(unparser): scope a sort key reading a projection output that cannot be repeated (refs spiceai/spiceai#14109) #247 (fix/14109-sort-key-volatile-scope) carries the same schema-rebuild hunk in rewrite_plan_for_sort_on_non_projected_fields, so whichever lands second drops a duplicate hunk. fix(unparser): list every relation a join input reads when the input lists no columns of its own (refs spiceai/spiceai#14738) #264 edits the Join and Aggregate arms only. A declined fold whose fetch keeps more than one row still leaves its ORDER BY inside the derived table, as on spiceai-55 (unparser: a declined sort fold leaves the ORDER BY inside a derived table with nothing outside it #265).

[Copilot]

🟡 Changes recommended

Projection schema mismatches and incomplete derived-scope handling can still produce incorrect results or invalid SQL.

4 open findings

• Reject grouped predicates before deriving the limit · New
• Reject duplicate derived output names before qualifier removal · New
• Name aggregate outputs at the derived-table boundary · New
• Align replacement projection schema with reordered expressions · New

What changed in this PR

This PR updates SQL unparsing to preserve outer ordering and row bounds when logical plans contain nested sorts and limits.

Changes:

• Merge compatible literal bounds and scope nested operations when needed.
• Repoint references across derived tables and broaden projection hoisting.
• Add 15 regression tests for nested ordering, limits, and aggregate scopes.

File	Description
datafusion/​sql/​tests/​cases/​plan_to_sql.rs	Adds regression coverage for nested sorts and limits.
datafusion/​sql/​src/​unparser/​rewrite.rs	Allows projection hoisting for subset outputs.
datafusion/​sql/​src/​unparser/​plan.rs	Preserves outer clauses and handles derived scopes.
datafusion/​sql/​src/​unparser/​ast.rs	Adds ordering-state and literal-bound helpers.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved scope and projection edge cases can emit invalid SQL or return incorrect results.

4 open findings

• Projection bypasses refusal for fetched sort under expression bound · New
• Case-insensitive output names can resolve to the wrong column · New
• Hidden qualifier retained in struct-field sort key · New
• Dropped computed sort output remains unresolved · New

4 resolved since last review

• Align replacement projection schema with reordered expressions
• Name aggregate outputs at the derived-table boundary
• Reject duplicate derived output names before qualifier removal
• Reject grouped predicates before deriving the limit

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Two unresolved expression-binding issues can cause fetched sorts to select incorrect rows.

5 open findings

• Qualify rewritten ORDER BY references to avoid alias collisions · New
• Use bottom-up substitution to preserve inserted expression scope · New
• Hidden qualifier retained in struct-field sort key
• Case-insensitive output names can resolve to the wrong column
• Projection bypasses refusal for fetched sort under expression bound

1 resolved since last review

• Dropped computed sort output remains unresolved

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The new derived-table scope can capture correlated outer references and silently return incorrect values.

5 open findings

• Prevent outer-reference capture by derived table aliases · New
• Qualify rewritten ORDER BY references to avoid alias collisions
• Hidden qualifier retained in struct-field sort key
• Case-insensitive output names can resolve to the wrong column
• Projection bypasses refusal for fetched sort under expression bound

1 resolved since last review

• Use bottom-up substitution to preserve inserted expression scope

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Remaining sort-key resolution errors can produce invalid SQL or select incorrect rows.

7 open findings

• Distinct blocks aggregate resolution for sort expressions · New
• Sort unprojection re-substitutes shadowed input references · New
• Prevent outer-reference capture by derived table aliases
• Qualify rewritten ORDER BY references to avoid alias collisions
• Hidden qualifier retained in struct-field sort key
• Case-insensitive output names can resolve to the wrong column
• Projection bypasses refusal for fetched sort under expression bound

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Interactions between derived-table scopes, observable ordering, and correlated references need final human verification.

6 open findings

• Distinct blocks aggregate resolution for sort expressions
• Prevent outer-reference capture by derived table aliases
• Qualify rewritten ORDER BY references to avoid alias collisions
• Hidden qualifier retained in struct-field sort key
• Case-insensitive output names can resolve to the wrong column
• Projection bypasses refusal for fetched sort under expression bound

1 resolved since last review

• Sort unprojection re-substitutes shadowed input references

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Remaining field-path and output-name handling can produce invalid SQL or select incorrect rows.

3 open findings

• Validate remaining path after consuming map keys · New
• Reject ambiguous valid relation prefixes · New
• Prevent incorrect sort unprojection with repeated output names · New

3 resolved since last review

• Account for outer references in shadow checks
• Decline fold when schema reconstruction fails
• Validate full nested field paths before relation prefix selection

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

The widened projection fold still permits incorrect sort-key binding and repeated evaluation of volatile outputs.

2 open findings

• Declared-name collision leaves ORDER BY referencing dropped output · New
• Aggregate normalization captures qualified sort replacement · New

3 resolved since last review

• Prevent incorrect sort unprojection with repeated output names
• Reject ambiguous valid relation prefixes
• Validate remaining path after consuming map keys

Previously missed (1)

In code that hasn't changed since last review

Widened fold duplicates non-repeatable volatile output

datafusion/​sql/​src/​unparser/​rewrite.rs:335

The widened fold can duplicate a retained volatile output. An outer Projection::try_new_with_schema([r, r], fields [first, second]) over Sort[u.a, fetch 1] → Projection[u.a, random() AS r, u.b] now passes the subset check and emits two independent random() AS r select items. Both plan outputs read the same computed r, so their values must be equal. The fallback field allows the repeated output through, while the repeatability guard checks only dropped outputs. Decline the fold when a non-repeatable inner output is repeated, and add a regression for this shape.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Remaining folding and ordering cases can select incorrect rows or re-evaluate volatile expressions.

6 open findings

• Volatile sort keys are re-evaluated during order restoration · New
• Folding duplicates nonrepeatable retained outputs · New
• Renamed plain-column outputs select the wrong sort expression · New
• Case-sensitive alias matching misses qualified-name collisions · New
• Qualified sort replacements lose qualifiers in projection scopes · New
• Case-sensitive outer references shadow local aliases · New

2 resolved since last review

• Aggregate normalization captures qualified sort replacement
• Declared-name collision leaves ORDER BY referencing dropped output

🧠 Review effort: Balanced

[spiceemma]

Stopping automated fix rounds on this PR. It needs a maintainer to decide how far the sort fold should reach.

What happened: today's runs pushed six rounds (8837afb9c → 3d8e76946 → 67bd2dc43 → ed3515dc5, plus earlier ones). Each round reproduced and fixed the open Copilot threads, and each new head then drew a fresh set of findings on other shapes the widened projection fold accepts. The latest review on ed3515dc5 has 6 open threads:

• volatile sort keys evaluated in both the inner and outer ORDER BY (plan.rs:2931)
• a retained volatile output repeated in the replacement projection (rewrite.rs:294)
• renamed plain-column outputs, where expression and field selection disagree (rewrite.rs:336)
• case-variant identifiers in the alias guard and the outer-reference guard (rewrite.rs:468, rewrite.rs:562)
• a qualified substitution that loses its qualifier in a projection-only scope (rewrite.rs:468)

These 6 threads are unverified, code inspection only (Copilot's). This run did not reproduce them and left them unresolved.

The question for a maintainer: keep adding a guard for each shape, or narrow the fold to a subset that is safe by construction? One such subset: every retained output is a plain, non-renamed column, nothing is volatile, and names compare case-folded. Anything else keeps the nested sort. Narrowing would close these findings as a class rather than one at a time. It would also give up some of the folds this PR enables today.

[grokspice]

Narrowing the sort fold. The user released the hold from the escalation comment above. This round pushed 7a0e80723, 56442fc82 and 05cd0b2c7, then merged spiceai-55 in bdd8d629b, because the base had advanced to ddd5c8ce6 (#261) and the PR had become CONFLICTING.

The decision. Stop adding a guard for each new shape. Make the widened fold safe by construction instead.

The fold has always hoisted a sort when the outer projection and the sort read exactly the inner projection's outputs (equal sets). This PR widened it to also drop inner outputs nothing above reads. That widening now runs only when all four hold:

• every output it keeps is a plain column named by that column, in both the inner and the outer schema;
• the outer list reads each kept output once;
• no plain column the inner projection renames is read above under the column's own name;
• no inner output is volatile or holds a subquery.

Any other shape keeps the sort in its own scope, and its SQL is byte-identical to spiceai-55. The capture guards that remain now compare names through Unparser::emitted_column_key, so they are case-folded.

No guard became dead, so none was removed. The equal-sets fold, which spiceai-55 already has, still reaches every one of them. I confirmed this with a mutation pass on the 7a0e80723 sources. Each row disables one guard; the right column lists the tests that then fail, beyond the 22 that already fail on the base.

disabled	fails
a key reads a name a repeat borrowed	test_a_repeated_output_taking_a_dropped_outputs_name_keeps_the_sort_in_its_scope
a key reads a dropped output that cannot be repeated	test_a_sort_key_reading_a_dropped_volatile_output_keeps_the_sort_in_its_scope
a kept alias would capture a substitution (alias and outer reference)	four tests, including …_in_another_case_unqualified_by_a_derived_aggregate_scope_keeps_the_sort and …_keeps_the_declared_name_the_fold_gives_it_in_the_shadow_guard
a repeated non-repeatable output	test_a_repeated_volatile_output_keeps_the_sort_in_its_scope
a derived projection counts as hiding the qualifier	test_a_qualified_substitution_unqualified_by_a_derived_projection_scope_keeps_the_sort
widened_fold_is_safe (fold always taken)	given-up folds 1 to 4 below, and test_a_renamed_plain_column_output_keeps_the_sort_in_its_scope
bare-key capture check	test_a_bare_key_a_computed_alias_in_another_case_would_capture_above_a_derived_aggregate_is_refused
volatile key re-established	test_an_expression_bound_above_a_sorted_chain_with_a_volatile_key_is_refused

Copilot's six threads on ed3515dc5. Each was reproduced on ed3515dc5, covered by a regression test, fixed in 7a0e80723, answered on its thread with the evidence, and resolved. The rows were run in SQLite 3.50.4 and DuckDB 1.4.4.

thread	ed3515dc5	bdd8d629b
a volatile key re-established above a sorted chain (plan.rs)	random() is drawn in both ORDER BYs; with deterministic draws the plan returns (10, 10) and the SQL returns (8, 8)	refused (NotImplemented)
a retained volatile output repeated	the two columns match in 0/20 runs on both engines	spiceai-55's SQL: DuckDB matches 20/20, SQLite 0/20, as SQLite already does on spiceai-55 (#267, item 3)
renamed plain-column outputs	returns 100; the plan returns 0	spiceai-55's SQL, which returns 1 because of a declared-name rendering gap already on the base (#267, item 1)
the alias guard compares case-sensitively	-2; the plan returns -1	-1 on both engines, and the equal-sets variant also gives -1
a qualifier lost in a projection-only scope	-2; the plan returns -1	-1 on both engines, and the equal-sets variant also gives -1
the outer-reference guard compares case-sensitively	-5; the plan returns -1	SQLite -1. DuckDB still returns -5, the same as on spiceai-55, because of its lateral alias binding (#267, item 2)

All six resolved; a re-query shows 0 of 39 review threads unresolved.

The bare-key capture from the earlier comment. It was not covered at ed3515dc5. Both plans unparsed to a bare ORDER BY s beside (0 - s) AS "S":

• case 1: SQLite and DuckDB both returned -2|2; the plan returns -1|1;
• case 2: SQLite returned -2|2 and DuckDB -1|1.

Since 7a0e80723, a bare key is refused when a computed alias whose name folds to the key's name would capture it. The one exception is the alias the key itself reads, spelled the same. The test is test_a_bare_key_a_computed_alias_in_another_case_would_capture_above_a_derived_aggregate_is_refused. Its control, a bare s over (0 - s) AS s, still unparses and returns the plan's -2|2.

Two findings from the adversarial review, both reproduced and fixed:

• 56442fc82. The exemption above is wrong for a sort absorbed from below the projection, whose key reads the derived table. ORDER BY s beside (0 - s) AS s returned -20 on both engines; the plan returns -10. spiceai-55 emits the same SQL. The absorbed case now gets no exemption.
• 05cd0b2c7. An outer reference in a re-established key was captured by a computed alias: DuckDB returned -3 and the plan returns -1. It now goes through the same capture check as the key's columns.

Folds given up. These existing PR tests now expect spiceai-55's SQL. Each plan keeps a computed or renamed output while dropping others. I checked each new expectation byte for byte against what spiceai-55 emits.

1. test_a_sort_folded_through_reordered_and_pruned_computed_aliases, renamed …_over_reordered_and_pruned_computed_aliases_keeps_its_scope.
   • Was: SELECT (u.b + 1) AS y, (u.a + 1) AS x FROM u ORDER BY ((u.a + 1) + 1) ASC NULLS FIRST LIMIT 1
   • Now: SELECT y, x FROM (SELECT (u.a + 1) AS x, (u.b + 1) AS y, u.c FROM u ORDER BY ((u.a + 1) + 1) ASC NULLS FIRST LIMIT 1)
   • SQLite and DuckDB return the plan's row.
2. test_a_fetched_sort_on_an_unnamed_computed_output_the_outer_projection_drops.
   • Was: SELECT u.a AS x FROM u ORDER BY (u.a + u.b) ASC NULLS FIRST LIMIT 1
   • Now: SELECT x FROM (SELECT u.a AS x, (u.a + u.b) AS "u.a + u.b", u.c FROM u ORDER BY "u.a + u.b" ASC NULLS FIRST LIMIT 1)
   • Both engines return the plan's row.
3. test_a_qualified_substitution_in_a_compound_key_over_a_derived_aggregate_still_folds, renamed …_keeps_the_sort.
   • Was: SELECT (0 - k) AS k FROM (SELECT t.k, t.v FROM t) GROUP BY k ORDER BY (k + 1) ASC NULLS FIRST LIMIT 1
   • Now: SELECT k FROM (SELECT (0 - k) AS k, k AS x, sum(v) AS s FROM (SELECT t.k, t.v FROM t) GROUP BY k ORDER BY (x + 1) ASC NULLS FIRST LIMIT 1)
   • Both engines return -1, the plan's answer. In an earlier round PostgreSQL 17 rejected this SQL with column "x" does not exist: an error, not wrong rows. I did not re-run it here.
4. test_a_repeated_output_taking_a_dropped_outputs_name_still_folds_a_key_not_reading_it, renamed …_keeps_the_sort_for_a_key_not_reading_it.
   • Was: SELECT (t.a + 1) AS x, (t.a + 1) AS x FROM t ORDER BY ((t.a + 1) + 1) ASC NULLS FIRST LIMIT 3
   • Now: SELECT x, x FROM (SELECT (t.a + 1) AS x, (t.b + 1) AS y, t.c AS unused FROM t ORDER BY ((t.a + 1) + 1) ASC NULLS FIRST LIMIT 3)
   • Both engines returned the plan's three rows in order, but SQL does not promise a derived table's order (unparser: a declined sort fold leaves the ORDER BY inside a derived table with nothing outside it #265).
5. test_an_unnamed_output_declared_under_another_name_is_substituted_by_its_field. The original plan kept the computed x, with t.d declared unused; it emitted SELECT (t.a + t.b) FROM t ORDER BY (t.c + 1) ASC NULLS FIRST LIMIT 1.
   • That fold is given up. On a debug build the nested path then trips the base's own debug_assert in name_derived_scope_outputs, exactly as spiceai-55 does for the same plan.
   • The test now keeps t.d under its own name, so it still pins substitution by declared field inside the subset: SELECT t.d FROM t ORDER BY (t.c + 1) ASC NULLS FIRST LIMIT 1.

Tests against the base. I compared failures by test name.

• sql_integration on spiceai-55 at ddd5c8ce6: 828 passed, 22 failed.
• sql_integration on this branch at bdd8d629b: 896 passed, 22 failed. The 22 are the same set by name, e.g. test_struct_expr and snowflake_flatten_*.
• The PR adds 68 tests. Run against the ed3515dc5 sources, the final test file fails 13 beyond the 22: the 9 regression tests for this round and the 4 given-up folds 1 to 4.
• --lib: 103 passed. cargo fmt --check: clean. clippy --all-targets -D warnings (1.97.0): clean.
• The base merge's only conflict was both sides appending tests to plan_to_sql.rs. I took fix(unparser): keep a union input's own ORDER BY and LIMIT inside that input #261's tests as they were and appended this PR's after them. fix(unparser): keep a union input's own ORDER BY and LIMIT inside that input #261's struct-field test now calls get_field through this PR's import instead of its full path, which the unused-qualification lint denies.

Review gate.

• codex job review-mvbma1d1-uijeaw reviewed 7a0e80723 and review-mvbmrj3w-21cd7u reviewed 56442fc82. Each returned one finding, fixed as described above.
• On 05cd0b2c7 and bdd8d629b the review was skipped: codex exited 1 with "Your workspace is out of credits".
• A read-only correctness pass over 05cd0b2c7 found no defects. It left two notes, both unverified and from code inspection only: a qualified outer reference is over-refused (a lost pushdown, never wrong rows), and an outer reference nested inside a compound absorbed key is not checked against computed aliases.
• /security-review: no findings.
• /simplify: three behavior-neutral proposals, applied in 05cd0b2c7.

Not verified here.

• PostgreSQL, which this host doesn't have.
• Release builds. The given-up declared-rename plan (fold 5) was only run with debug assertions on.

refs spiceai/spiceai#14733

[Copilot]

🔵 Needs a closer look

Interacting scope and alias rewrites require human review, with two unresolved correctness issues remaining.

2 open issues

• Avoid ambiguous display names when folding projections · New
• Respect declared schema names for aliased projection outputs · New

6 resolved since last review

• Case-sensitive outer references shadow local aliases
• Qualified sort replacements lose qualifiers in projection scopes
• Case-sensitive alias matching misses qualified-name collisions
• Renamed plain-column outputs select the wrong sort expression
• Folding duplicates nonrepeatable retained outputs
• Volatile sort keys are re-evaluated during order restoration

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Projection folding can leave a surviving sort key bound to a source column instead of the aggregate output, selecting incorrect rows.

1 open issue

• Projection folding loses aggregate alias binding in ORDER BY · New

2 resolved since last review

• Respect declared schema names for aliased projection outputs
• Avoid ambiguous display names when folding projections

_{Review effort: Balanced}

[grokspice]

Merge conflict with spiceai-55 needs a maintainer decision. #246 and #247 merged at 03:31Z, and this PR is now DIRTY. I pushed b7f24d2d6, the fix for the latest Copilot thread (details on that thread), and did not push a merge of spiceai-55.

What I tried. In a scratch worktree I merged spiceai-55 0e01a882f into b7f24d2d6. There were 9 conflict hunks in ast.rs, utils.rs, plan.rs, rewrite.rs and plan_to_sql.rs:

• Most were mechanical: imports, append/append tests, and fix(unparser): keep the ORDER BY and row bound a node above already set, instead of letting a nested sort or limit overwrite them (refs spiceai/spiceai#14733) #263's richer copy of the fold's field-building block that fix(unparser): scope a sort key reading a projection output that cannot be repeated (refs spiceai/spiceai#14109) #247 also added.
• It also needed one non-textual adaptation: fix(unparser): keep a qualifier that an UNNEST or FLATTEN alias, a schema-qualified table or a struct field path binds (refs spiceai/spiceai#13442) #246 changed enclosed_qualifiers/requalify_column_onto_derived_table to take EnclosedRelations, and fix(unparser): keep the ORDER BY and row bound a node above already set, instead of letting a nested sort or limit overwrite them (refs spiceai/spiceai#14733) #263's repoint_onto_derived_chain still passed a HashSet.

The merged tree builds and is clippy-clean. Under RUST_MIN_STACK=16777216 (the default stack overflows in roundtrip_statement on this host), sql_integration gives 985 passed, 5 failed. Each failure is a test from one PR that pins the opposite behaviour to the other PR:

test	from	merged tree emits	the test expects
test_a_sort_key_reading_a_dropped_volatile_output_keeps_the_sort_in_its_scope	#263	… FROM (SELECT u.a, random() AS r, u.c FROM u) ORDER BY (((r - r) * 100.0) + a) …	random() inlined into the key
test_sort_key_reading_a_volatile_output_the_list_above_drops_is_inlined_once	#247	a derived scope, ORDER BY (r + 1.0)	flat ORDER BY (random() + 1.0)
test_sort_key_reading_a_volatile_output_under_a_limit_below_another_sort_is_scoped	#247	NotImplemented: … row limit in a scope of its own …	unparsed
test_fetched_sort_key_reading_a_volatile_output_in_a_join_input_is_refused	#247	refused with #263's join-input row-limit message	refused with #247's volatile-key message
test_a_flattened_field_path_key_two_relation_prefixes_can_read_is_refused	#263	panics (plan_to_sql.rs, field-path refusal)	refused

The decision this needs. #247 (merged) and this PR each scope a sort key that reads a volatile or dropped output, in the same Sort arm and the same fold, and they disagree on which shape gets which SQL. #246 and this PR both handle the qualifier of a field-path key in a derived scope. Settling the merge means deciding, per shape, whose behaviour stands. I don't want to guess that, because a wrong resolution here produces SQL that is valid but returns different rows.

One way through is to treat #247's volatile-key scoping as authoritative, since it is the one on spiceai-55, and drop or re-expect #263's overlapping guards. The field-path case then needs #263's repoint_onto_derived_chain checked against #246's is_field_path requalification.

[Copilot]

🟡 Changes recommended

Unresolved sort-key binding and repeated-evaluation errors can select incorrect rows.

3 open issues

• Prevent repeated evaluation of volatile retained outputs · New
• Resolve projection aliases before aggregate outputs in sort keys · New
• Traverse foldable filters when resolving sort key scope · New

1 resolved since last review

• Projection folding loses aggregate alias binding in ORDER BY

_{Review effort: Balanced}