Implement ORDER BY and LIMIT/OFFSET instead of silently ignoring them#116
Conversation
exec_select accepted the parsed ORDER BY and LIMIT clauses but never used them, so 'SELECT ... ORDER BY x LIMIT 10' returned every matching row in unspecified order - a silent correctness violation and an unbounded result-size footgun. SELECT results are now sorted (numeric-aware comparison, multi-key, ASC/DESC, NULLS FIRST/LAST with SQL-standard defaults) and bounded by LIMIT/OFFSET. Distributed reads apply the clauses after the cluster merges replica results so cross-node queries honor them too. Clause forms the engine cannot honor (ORDER BY arbitrary expressions, LIMIT BY, WITH FILL, INTERPOLATE) are rejected with an Unsupported error rather than ignored. https://claude.ai/code/session_01CJXWjB9ERGgV6B9mRqrdM9
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e66ecc9457
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Meta results are merged across replicas by the cluster layer, | ||
| // which applies ORDER BY/LIMIT after the merge. | ||
| if let QueryOutput::Rows(ref mut rows) = out { | ||
| apply_order_and_limit(rows, order.as_ref(), limit)?; |
There was a problem hiding this comment.
When the query projects only a subset of columns and orders by a different column, this sorts the already-projected rows, so the ORDER BY key has been removed by project_row and every row compares as None. For example, SELECT ck FROM items WHERE pk = 'p' ORDER BY score will keep storage/key order instead of score order; the clustered path has the same issue because meta rows encode the projected map. Either sort before projection or reject ORDER BY columns that are not present in the result rows.
Useful? React with 👍 / 👎.
2 participants
Problem
exec_selectaccepted the parsedORDER BYandLIMITclauses but bound them to_order/_limitand never used them.SELECT ... ORDER BY x LIMIT 10returned every matching row in unspecified order — a silent correctness violation and an unbounded result-size footgun.Fix
New
query::apply_order_and_limitapplied to SELECT results:2 < 10),NULLS FIRST/LASTwith the SQL-standard defaults (nulls last ascending, first descending).LIMIT n [OFFSET m]and the MySQLLIMIT m, nform; offset past the end yields empty rather than erroring.ORDER BYarbitrary expressions,LIMIT BY,WITH FILL,INTERPOLATE) now returnQueryError::Unsupportedinstead of being ignored.Tests
tests/query_order_limit_test.rs: ascending/descending, numeric ordering, multi-column sort with mixed directions, LIMIT truncation incl.LIMIT 0, OFFSET paging incl. past-the-end, missing-column rows sorting last, and rejection of unsupported ORDER BY expressions.Found by codebase audit (medium severity: silent correctness violation).
https://claude.ai/code/session_01CJXWjB9ERGgV6B9mRqrdM9
Generated by Claude Code