Skip to content

fix(engine): resolve ORDER BY <ordinal> to the Nth output column (P16) - #59

Merged
TimelordUK merged 1 commit into
mainfrom
fix/p16-order-by-ordinal
Aug 31, 2026
Merged

fix(engine): resolve ORDER BY <ordinal> to the Nth output column (P16)#59
TimelordUK merged 1 commit into
mainfrom
fix/p16-order-by-ordinal

Conversation

@TimelordUK

Copy link
Copy Markdown
Owner

Closes P16. Parity 134 → 139 AGREE (+2 fixed, +3 new coverage, +1 new BOTH_ERR); contract holds at 168 cases.

The defect

ORDER BY 2 returned rows in natural insertion order, with no error.

The integer was not being ignored. OrderByAliasTransformer promotes any ORDER BY item that isn't a visible column into a hidden SELECT item, so the key survives projection — and NumberLiteral("2") qualified. The query then sorted, correctly, on a hidden column whose value is the constant 2, where every row compares equal.

Row count gives no hint: ORDER BY 2 DESC LIMIT 3 returns three rows that look right but are the first three in file order.

Where the fix goes, and why

The ordinal is positional against the output columns, so it is resolved in query_engine::apply_multi_order_by_with_context, not in the transformer. The transformer knows the select list but not the output — SELECT * is still unexpanded there, and GROUP BY hasn't run — and both are shapes where ORDER BY 2 must keep meaning the same thing. The transformer's only job is to stop promoting numeric literals.

Columns promoted for ORDER BY visibility (and HAVING's __hidden_agg_) are appended after the real output, so they're excluded from the ordinal range: SELECT a, b FROM t ORDER BY c, 3 errors rather than landing on __hidden_orderby_1.

Rules, pinned against DuckDB before implementing

Query Behaviour
ORDER BY 2 2nd output column — explicit select list, SELECT *, or after GROUP BY
ORDER BY 1+1 not an ordinal: an ordinary constant expression, sorts nothing
ORDER BY 0 / -1 / 3 of 2 error, should be between 1 and N
ORDER BY 1.5 error — DuckDB: "ORDER BY non-integer literal has no effect"

That last row is why the transformer skips every numeric literal, not just integer-valued ones: promoting 1.5 would leave it a silent no-op, and only the engine knows the valid range to report. Same principle as P13 stage 1 — a refusal beats a different query that succeeds.

Coverage

Four new corpus cases beyond the two that were pinned. The defect is about output columns, so each construct that changes what those are is a separate risk — order_by_ordinal_group_by matters most in practice, since "top N by total" is the shape where silently returning group order was most likely to be believed.

Plus 3 transformer unit tests and 6 engine integration tests, including one asserting hidden promoted columns stay out of the ordinal range.

Verification

  • Parity: 139 AGREE / 13 DIFFER / 14 GAP / 2 BOTH_ERR, contract holds (168 cases)
  • cargo test: 734 + 461 + 1 passed, 0 failed
  • cargo fmt, cargo clippy (7 pre-existing errors on main, unchanged)
  • Examples: all FORMAL pass; SMOKE failures pre-existing (offline URLs, missing fixtures)
  • Python: 7 failed / 527 passed — the known Windows .exe-suffix noise, same set as the recorded baseline

🤖 Generated with Claude Code

https://claude.ai/code/session_01TmQRCdZUn3RYqyVFoeKRsY

`ORDER BY 2` returned rows in natural insertion order, with no error. The
integer was not being ignored — `OrderByAliasTransformer` promotes any ORDER BY
item that is not a visible column into a hidden SELECT item so it survives
projection, and `NumberLiteral("2")` qualified. So the query sorted, correctly,
on a hidden column whose value is the constant 2, where every row compares equal.

Row count gives no hint of this: `ORDER BY 2 DESC LIMIT 3` returns three rows
that look plausible but are the first three in file order.

The ordinal is positional against the *output* columns, so it is resolved in
`query_engine::apply_multi_order_by_with_context` rather than in the
transformer. The transformer knows the select list but not the output: `SELECT *`
is still unexpanded there and GROUP BY has not run, and both are shapes where
`ORDER BY 2` has to keep meaning the same thing. The transformer's only job is
to stop promoting numeric literals.

Rules pinned against DuckDB before implementing, not assumed:

- `ORDER BY 2` — 2nd output column, under an explicit select list, `SELECT *`,
  or after GROUP BY.
- `ORDER BY 1+1` — NOT an ordinal. It is an ordinary constant expression and
  sorts nothing, in both engines.
- `ORDER BY 0` / `-1` / out of range — error.
- `ORDER BY 1.5` — error. This is why the transformer skips every numeric
  literal rather than only integer-valued ones: promoting `1.5` would leave it a
  silent no-op, and only the engine knows the valid range to report. Same
  principle as P13 stage 1 — a refusal beats a different query that succeeds.

Columns promoted for ORDER BY visibility (and HAVING's `__hidden_agg_` columns)
are appended after the real output, so they are excluded from the ordinal range:
`SELECT a, b FROM t ORDER BY c, 3` errors rather than landing on
`__hidden_orderby_1`.

Parity 134 -> 139 AGREE (+2 fixed, +3 new coverage, +1 new BOTH_ERR).
`order_by_ordinal_star` and `order_by_ordinal_group_by` were added because the
defect is about output columns, so each construct that changes what those are is
a separate risk — and "top N by total" is where silently returning group order
was most likely to be believed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TmQRCdZUn3RYqyVFoeKRsY
@TimelordUK
TimelordUK merged commit 4a4eef8 into main Aug 31, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant