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

docs/SQL_PARITY.md

@@ -82,7 +82,8 @@ Suggested fix order, by silent blast radius:
 | ~~4~~ | ~~[P28](#p28) `INTO #tmp` stages unfiltered rows~~ | ✅ **Fixed 2026-08-16** — turned out to stage the *whole source table*, and the sweep it prescribed found [P31](#p31) |
 | ~~7~~ | ~~[P18](#p18)/[P19](#p19) three-valued logic~~ | ✅ **Fixed 2026-08-22** — 125 → 129 AGREE. Delivered as three slices ([R10](ENGINE_REFACTORING.md#r10)); the two no-op ones landed first, so the semantics change reviewed on its own |
 | ~~8~~ | ~~[P24](#p24) `RANGE` treated as `ROWS`~~ | ✅ **Fixed 2026-08-30** — 129 → 133 AGREE (+2 fixed, +2 new coverage). One defect, not two: the parser already emitted the right default frame, so fixing peer groups closed both cases. Spun off [P33](#p33) |
-| 9 | [P14](#p14), [P16](#p16), [P17](#p17), [P20](#p20), [P23](#p23), P13 stage 2 | Smaller, self-contained, decisions already taken |
+| ~~9a~~ | ~~[P16](#p16) `ORDER BY <ordinal>` ignored~~ | ✅ **Fixed 2026-08-31** — 134 → 139 AGREE. The literal was being promoted into a hidden *constant* column, so the sort ran on a column where every row tied |
+| 9b | [P14](#p14), [P17](#p17), [P20](#p20), [P23](#p23), P13 stage 2 | Smaller, self-contained, decisions already taken |
 | 10 | [P22](#p22), [P25](#p25), [P26](#p26), [P15](#p15), [P32](#p32) | Hard errors — visible, so less urgent than any of the above |
 | — | [P27](#p27) `OR` in `JOIN ... ON` | **Reclassified 2026-08-08 — not a quick win.** `JoinCondition` is a `Vec<SingleJoinCondition>` implicitly AND-ed, so there is nowhere in the AST to put an `OR`; it needs join conditions to become an expression, which reaches the join execution code. Sequence it with the R-log, not here |
 
@@ -629,9 +630,13 @@ annotation be removed.
   silently or loudly depending only on luck.
 
 ### P16 — `ORDER BY <ordinal>` is silently ignored
-- **Status:** 🔴 OPEN
+- **Status:** 🟢 FIXED 2026-08-31 — 134 → 139 AGREE (+2 fixed, +3 new coverage,
+  +1 new BOTH_ERR)
 - **Corpus:** `08_ordering.toml :: order_by_ordinal`, `order_by_ordinal_desc`
-  (both DIFFER).
+  (were DIFFER, now AGREE). Added with the fix: `order_by_ordinal_star`,
+  `order_by_ordinal_group_by`,
+  `order_by_ordinal_expression_is_not_positional` (AGREE) and
+  `order_by_ordinal_out_of_range` (BOTH_ERR).
 - **Observed:** `ORDER BY 2` and `ORDER BY 2 DESC` return rows in **natural
   insertion order** — no sorting is applied at all, and no error is raised. The
   integer is evaluated as a constant expression, so every row compares equal.
@@ -649,6 +654,53 @@ annotation be removed.
   were the first three in file order rather than the top three. Row count is not
   evidence of correct ordering.
 
+#### As built (2026-08-31)
+
+The root cause was not in the sort at all, and not where the entry above guessed
+("the integer is evaluated as a constant expression"). It is close, but the
+mechanism matters for where the fix goes:
+`OrderByAliasTransformer::promote_hidden_order_by_columns` exists to keep an
+ORDER BY key alive through projection, and it promotes **anything that is not a
+visible column** into a hidden SELECT item. `NumberLiteral("2")` is not a
+column, so it was promoted as a hidden column *whose value is the constant 2* —
+after which the engine sorted, correctly, on a column where every row compares
+equal. Nothing was ignored; the wrong thing was sorted on.
+
+**The ordinal is resolved during execution, not in the transformer.** The
+transformer knows the select list but not the *output* columns, and the two
+differ in exactly the cases that matter: `SELECT *` is still unexpanded there,
+and GROUP BY has not run. `apply_multi_order_by_with_context` in
+`query_engine.rs` sees the projected view and resolves all three shapes with one
+rule, so the transformer's only job is to stop promoting numeric literals.
+
+Rules pinned against DuckDB before implementing, rather than assumed:
+
+| Query | Behaviour |
+|---|---|
+| `ORDER BY 2` | 2nd output column — under an explicit select list, `SELECT *`, or after GROUP BY |
+| `ORDER BY 1+1` | **not** an ordinal: an ordinary constant expression, sorts nothing |
+| `ORDER BY 0`, `ORDER BY -1`, `ORDER BY 3` (of 2) | error, `should be between 1 and N` |
+| `ORDER BY 1.5` | error — DuckDB: *"ORDER BY non-integer literal has no effect"* |
+
+The last row is why the transformer skips **every** numeric literal and not just
+integer-valued ones: leaving `1.5` to be promoted would have kept it a silent
+no-op, and only the engine knows the valid range to report. That is the P13
+principle — a refusal beats a different query that succeeds.
+
+**Hidden columns are excluded from the ordinal range.** Columns promoted for
+ORDER BY visibility (and HAVING's `__hidden_agg_` columns) are appended *after*
+the real output, so `SELECT a, b FROM t ORDER BY c, 3` must error rather than
+resolve to `__hidden_orderby_1`. Pinned by
+`test_order_by_ordinal_ignores_promoted_hidden_columns`.
+
+**A note on picking the corpus cases.** Two cases would have been enough to turn
+the entry green, and would have left the two most valuable shapes untested: the
+defect is about *output* columns, so every construct that changes what the
+output columns are is a separate risk. `SELECT *` and GROUP BY were added for
+that reason, and `order_by_ordinal_group_by` is the one that matters most in
+practice — "top N by total" is where silently returning group order is most
+likely to be believed.
+
 ### P17 — Default NULL placement differs on `ASC`
 - **Status:** 🔴 OPEN — **decision made 2026-08-02: follow the reference engine
   (NULLS LAST in both directions), plus explicit `NULLS FIRST`/`LAST` from P13

src/data/query_engine.rs

@@ -3135,6 +3135,55 @@ impl QueryEngine {
         Ok(view.with_rows(unique_row_indices))
     }
 
+    /// Resolve `ORDER BY <n>` - a 1-based positional reference to a select-list
+    /// item - to a source column index (P16).
+    ///
+    /// The reference is against the *output* columns, so it is resolved here
+    /// rather than in `OrderByAliasTransformer`: by this point projection,
+    /// `SELECT *` expansion and GROUP BY have all run, and `ORDER BY 2` means
+    /// the same thing in every one of those shapes.
+    ///
+    /// Columns promoted for ORDER BY visibility (and HAVING's hidden
+    /// aggregates) are appended *after* the real output, so they are excluded
+    /// from both the mapping and the range check - `ORDER BY 3` must not
+    /// silently land on `__hidden_orderby_1`.
+    ///
+    /// Following DuckDB: out-of-range is an error, and a non-integer literal is
+    /// an error too rather than a sort that quietly does nothing.
+    fn resolve_order_by_ordinal(view: &DataView, literal: &str) -> Result<usize> {
+        use crate::query_plan::having_alias_transformer::HIDDEN_AGG_PREFIX;
+        use crate::query_plan::order_by_alias_transformer::HIDDEN_ORDERBY_PREFIX;
+
+        let output_columns: Vec<usize> = view
+            .get_display_columns()
+            .into_iter()
+            .filter(|&idx| {
+                view.source().columns.get(idx).is_none_or(|c| {
+                    !c.name.starts_with(HIDDEN_ORDERBY_PREFIX)
+                        && !c.name.starts_with(HIDDEN_AGG_PREFIX)
+                })
+            })
+            .collect();
+
+        let ordinal: i64 = literal.trim().parse().map_err(|_| {
+            anyhow!(
+                "ORDER BY {} has no effect: a non-integer literal is not a positional reference. Use a column name, or a position between 1 and {}",
+                literal,
+                output_columns.len()
+            )
+        })?;
+
+        if ordinal < 1 || ordinal as usize > output_columns.len() {
+            return Err(anyhow!(
+                "ORDER BY position {} is out of range - should be between 1 and {}",
+                ordinal,
+                output_columns.len()
+            ));
+        }
+
+        Ok(output_columns[ordinal as usize - 1])
+    }
+
     /// Apply multi-column ORDER BY sorting to the view
     fn apply_multi_order_by(
         &self,
@@ -3155,6 +3204,15 @@ impl QueryEngine {
         let mut sort_columns = Vec::new();
 
         for order_col in order_by_columns {
+            // ORDER BY <ordinal> is positional, so it resolves against the
+            // projected output rather than by name (P16).
+            if let SqlExpression::NumberLiteral(literal) = &order_col.expr {
+                let col_index = Self::resolve_order_by_ordinal(&view, literal)?;
+                let ascending = matches!(order_col.direction, SortDirection::Asc);
+                sort_columns.push((col_index, ascending));
+                continue;
+            }
+
             // Extract column name from expression (currently only supports simple columns)
             let (column_name, is_quoted) = match &order_col.expr {
                 SqlExpression::Column(col_ref) => (

src/query_plan/order_by_alias_transformer.rs

@@ -161,6 +161,17 @@ impl OrderByAliasTransformer {
         }
     }
 
+    /// Is this ORDER BY item a bare numeric literal?
+    ///
+    /// Only a bare literal counts. `ORDER BY 1+1` is an ordinary constant
+    /// expression that sorts nothing, which is what DuckDB does too, so it is
+    /// deliberately not matched here. Non-integer literals are matched so that
+    /// `ORDER BY 1.5` reaches the engine and is rejected there, rather than
+    /// being promoted into a constant column and silently sorting nothing.
+    fn is_numeric_literal(expr: &SqlExpression) -> bool {
+        matches!(expr, SqlExpression::NumberLiteral(_))
+    }
+
     /// Check if an ORDER BY column matches an aggregate pattern
     /// Returns the normalized aggregate string if it matches
     fn extract_aggregate_from_order_column(column_name: &str) -> Option<String> {
@@ -333,6 +344,16 @@ impl OrderByAliasTransformer {
                 }
             }
 
+            // ORDER BY <ordinal> (P16) is a positional reference to an output
+            // column, not an expression to compute. Promoting it would append a
+            // hidden *constant* column and sort on that - every row compares
+            // equal, so the sort silently becomes a no-op. Resolution needs the
+            // projected column list, which only exists once the query runs, so
+            // leave the literal in place for query_engine to resolve.
+            if Self::is_numeric_literal(&order_col.expr) {
+                continue;
+            }
+
             // Determine the dedup key and clone the expression we'll promote.
             // For Column refs we use the column name (case-insensitive); for
             // arbitrary expressions we use the debug-formatted string. Two
@@ -381,7 +402,7 @@ impl OrderByAliasTransformer {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::sql::parser::ast::{ColumnRef, QuoteStyle, SortDirection};
+    use crate::sql::parser::ast::{ColumnRef, OrderByItem, QuoteStyle, SortDirection};
 
     #[test]
     fn test_extract_aggregate_from_order_column() {
@@ -424,6 +445,99 @@ mod tests {
         );
     }
 
+    #[test]
+    fn ordinal_literal_is_not_promoted_to_a_hidden_column() {
+        // P16: promoting `ORDER BY 2` would append a hidden CONSTANT column and
+        // sort on it, so every row compares equal and the sort silently does
+        // nothing. The literal must survive to query_engine, which resolves it
+        // positionally against the projected output.
+        let mut stmt = SelectStatement::default();
+        stmt.select_items = vec![
+            SelectItem::Column {
+                column: ColumnRef::unquoted("id".to_string()),
+                leading_comments: Vec::new(),
+                trailing_comment: None,
+            },
+            SelectItem::Column {
+                column: ColumnRef::unquoted("score".to_string()),
+                leading_comments: Vec::new(),
+                trailing_comment: None,
+            },
+        ];
+        stmt.order_by = Some(vec![OrderByItem {
+            expr: SqlExpression::NumberLiteral("2".to_string()),
+            direction: SortDirection::Desc,
+        }]);
+
+        let stmt = OrderByAliasTransformer::new()
+            .transform_statement(stmt)
+            .expect("transform");
+
+        assert_eq!(stmt.select_items.len(), 2, "no hidden column appended");
+        assert!(matches!(
+            stmt.order_by.as_ref().unwrap()[0].expr,
+            SqlExpression::NumberLiteral(ref n) if n == "2"
+        ));
+    }
+
+    #[test]
+    fn non_integer_literal_also_reaches_the_engine() {
+        // `ORDER BY 1.5` is not positional, but it must still not be promoted:
+        // the engine rejects it, whereas a hidden constant column would make it
+        // a silent no-op. Only the engine knows the valid range to report.
+        let mut stmt = SelectStatement::default();
+        stmt.select_items = vec![SelectItem::Column {
+            column: ColumnRef::unquoted("id".to_string()),
+            leading_comments: Vec::new(),
+            trailing_comment: None,
+        }];
+        stmt.order_by = Some(vec![OrderByItem {
+            expr: SqlExpression::NumberLiteral("1.5".to_string()),
+            direction: SortDirection::Asc,
+        }]);
+
+        let stmt = OrderByAliasTransformer::new()
+            .transform_statement(stmt)
+            .expect("transform");
+
+        assert_eq!(stmt.select_items.len(), 1);
+        assert!(matches!(
+            stmt.order_by.as_ref().unwrap()[0].expr,
+            SqlExpression::NumberLiteral(_)
+        ));
+    }
+
+    #[test]
+    fn arithmetic_in_order_by_is_still_promoted() {
+        // The other half of the rule: `1+1` is an ordinary constant expression,
+        // not an ordinal, so it keeps the existing promotion path. Guards
+        // against the ordinal check widening to any numeric-valued expression.
+        let mut stmt = SelectStatement::default();
+        stmt.select_items = vec![SelectItem::Column {
+            column: ColumnRef::unquoted("id".to_string()),
+            leading_comments: Vec::new(),
+            trailing_comment: None,
+        }];
+        stmt.order_by = Some(vec![OrderByItem {
+            expr: SqlExpression::BinaryOp {
+                left: Box::new(SqlExpression::NumberLiteral("1".to_string())),
+                op: "+".to_string(),
+                right: Box::new(SqlExpression::NumberLiteral("1".to_string())),
+            },
+            direction: SortDirection::Asc,
+        }]);
+
+        let stmt = OrderByAliasTransformer::new()
+            .transform_statement(stmt)
+            .expect("transform");
+
+        assert_eq!(stmt.select_items.len(), 2, "expression promoted as hidden");
+        assert!(matches!(
+            stmt.order_by.as_ref().unwrap()[0].expr,
+            SqlExpression::Column(ref c) if c.name.starts_with(HIDDEN_ORDERBY_PREFIX)
+        ));
+    }
+
     #[test]
     fn test_is_aggregate_function() {
         let sum_expr = SqlExpression::FunctionCall {

tests/comparison/corpus/08_ordering.toml

@@ -125,24 +125,63 @@ expect = "BOTH_ERR"
 # is a table alias, which is valid SQL and accepted by the reference engine too.
 # The defect was only ever about tokens with nowhere to belong.
 
-# --- P16: ORDER BY <ordinal> is silently ignored ---
+# --- P16: ORDER BY <ordinal> (fixed 2026-08-31) ---
+#
+# The ordinal is positional — it names the Nth *output* column — so every shape
+# that changes what the output columns are needs its own case: an explicit
+# select list, SELECT *, and GROUP BY. Before the fix the literal was promoted
+# into a hidden CONSTANT column and sorted on, so all of them silently returned
+# insertion order with no error.
+#
+# All six cases filter NULLs out of the sort key, so a P17 regression cannot be
+# mistaken for a P16 one.
 
 [[case]]
 id = "order_by_ordinal"
 data = "null_edges.csv"
 sql = "SELECT id, score FROM null_edges WHERE score IS NOT NULL ORDER BY 2, 1"
-expect = "DIFFER"
-# P16. Rows come back in natural (insertion) order — the ordinal is evaluated as
-# a constant, so every row compares equal and nothing sorts. No error is raised.
-# NULLs are filtered out so this cannot be confused with P17.
+# P16. Resolves to the 2nd select-list item.
 
 [[case]]
 id = "order_by_ordinal_desc"
 data = "null_edges.csv"
 sql = "SELECT id, score FROM null_edges WHERE score IS NOT NULL ORDER BY 2 DESC, 1"
-expect = "DIFFER"
-# P16 with a direction, which is also ignored. Worth pinning separately: a fix
-# that resolves the ordinal but drops ASC/DESC would still pass the case above.
+# P16 with a direction. Worth pinning separately: a fix that resolved the
+# ordinal but dropped ASC/DESC would still pass the case above.
+
+[[case]]
+id = "order_by_ordinal_star"
+data = "null_edges.csv"
+sql = "SELECT * FROM null_edges WHERE score IS NOT NULL ORDER BY 3, 1"
+# P16 under SELECT *, where the ordinal counts source columns rather than
+# select-list items. This is why the ordinal is resolved during execution and
+# not in OrderByAliasTransformer — the star is still unexpanded there.
+
+[[case]]
+id = "order_by_ordinal_group_by"
+data = "null_edges.csv"
+sql = "SELECT team, SUM(score) AS total FROM null_edges WHERE team IS NOT NULL AND score IS NOT NULL GROUP BY team ORDER BY 2 DESC, 1"
+# P16 after GROUP BY, where the ordinal counts the aggregated output. The
+# "top N by total" shape, and the one where silently returning group order was
+# most likely to be believed.
+
+[[case]]
+id = "order_by_ordinal_expression_is_not_positional"
+data = "null_edges.csv"
+sql = "SELECT id, score FROM null_edges WHERE score IS NOT NULL ORDER BY 1+1, 1"
+# The other half of the rule: only a BARE integer is positional. `1+1` is an
+# ordinary constant expression that sorts nothing, in both engines. Pinned so a
+# future "evaluate ORDER BY expressions" change cannot quietly turn arithmetic
+# into an ordinal.
+
+[[case]]
+id = "order_by_ordinal_out_of_range"
+data = "null_edges.csv"
+sql = "SELECT id, score FROM null_edges ORDER BY 3"
+expect = "BOTH_ERR"
+# Out of range is an error, not a no-op — same reasoning as P13 stage 1. The
+# range excludes columns promoted for ORDER BY visibility, which are appended
+# after the real output: `ORDER BY label, 3` must not land on the hidden one.
 
 # --- P17: default NULL placement differs on ASC ---