[tonyalaribe]

DataFusion rejects SELECT k, round(avg(v)) FROM t GROUP BY 1 ORDER BY 2 with "Column in ORDER BY must be in GROUP BY or an aggregate function", but accepts the same item spelled out. Postgres accepts both. On prod, per-route p95 queries such as approx_percentile(0.95, percentile_agg(duration)) ordered by 2 failed this way.

• The existing pre-planning AST rewrite (today it handles row_to_json) now replaces an ORDER BY <n> ordinal with its select item when all of these hold:
  • the query has a GROUP BY;
  • the item is an unaliased expression;
  • the item is not a group key.
• Group keys are untouched, so GROUP BY 1 ORDER BY 1 time-bucket charts keep their exact SQL.
• Unions are untouched, because their ordinals name the union's output.
• A cheap regex guard keeps other statements off the parse/unparse path.

Tests:

• a case table over a real DataFusion session, covering the rewritten cases and the declined cases;
• order_by_ordinal_over_a_scalar_aggregate_plans_on_both_protocols, a pgwire test on both the simple and extended protocols.

Both fail with the rewrite disabled (red arm checked).

Local signoff (make ci-signoff on 64ae142): fmt, clippy, test, pg-smoke and e2e all pass and are attested.

🤖 Generated with Claude Code

[monoscope-bot]

Monoscope production impact · Worth checking

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 0dc93f84ff72e1809bf4ee831a03ebd560555320 · advisory

Evidence window (UTC): 2026-10-19 10:15:26.755286388 UTC → 2026-10-26 10:15:26.755286388 UTC

src/read/optimizers.rs:2008 (Head)

Production details are available in Monoscope.

telemetry evidence

src/read/optimizers.rs:2008 (Head)

Production details are available in Monoscope.

telemetry evidence

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

[claude]

Review

Overall this is a small, well-scoped fix. The case table runs against a real DataFusion session, there is a pgwire test on both protocols, and the red arm was checked. I found one correctness gap and a few smaller points.

Possible bug: wildcards break the ordinal-to-projection mapping (rewrite_order_ordinals)

• select.projection.get(n - 1) assumes one select item per output column.
• With SELECT *, round(avg(v)) FROM t GROUP BY ... ORDER BY 2, or t.* ahead of the item, * expands to several columns. Ordinal n then names a different column than projection[n-1].
• The rewrite would silently sort by the wrong expression, and no error would flag it.
• Suggested fix: bail out when any item at or before index n-1 is Wildcard or QualifiedWildcard. Add a case-table row for it.

Minor

• Substituting the expression duplicates evaluation of volatile items such as random() in SELECT k, random()*count(*) ... ORDER BY 2. That is probably acceptable, but it is worth a one-line comment or a decline for volatile functions.
• SelectItem::ExprWithAlias is skipped. If round(avg(v)) AS r ... ORDER BY 2 fails the same way in DataFusion, it needs the same treatment (substitute the expression, or the alias). If it already plans, a test row would pin that.
• The might_need_rewrite regex is a loose guard. [^;]*?,\s* can match across LIMIT 5, ... or window ORDER BYs. That only costs an extra parse/unparse, so it is fine. A short comment saying false positives are intentional would help.
• LazyLock<Regex> is built inside the function. This is fine, but it could sit at module level next to the other statics if there are any.
• The ordinal closure's n.parse::<usize>() returns None for 0 or an overflowing number, which are left to DataFusion. checked_sub handles 0. Good.

Tests

• Coverage is good. Please add the wildcard decline case.
• Consider a nested-subquery case (ORDER BY ordinal inside a CTE or subquery), since post_visit_query handles those too.

Nothing security-relevant. The change is purely a planner-side AST rewrite.