Repository navigation
feat(engine): refuse a parameter as a whole sort or group key - #42
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
On SQLite, a position or alias that names a result column which is a bare parameter is now refused as reylo14. The error lands in the select list and quotes a clause the author never wrote. Postgres and MySQL accept the same statement.
Reviewed changes
Reviewed both commits: the new reylo14 diagnostic across the three engines, the oracle and TS tests, and the docs.
reylo14/CONSTANT_KEY:constant_keyinengine/corebuilds a message for each clause (one wording when the key is alone, another beside other keys). Each dialect reports it at the placeholder and still types the statement.- Postgres: re-parses the rewritten text with libpg_query. It walks
SortBy,group_clause,distinct_clauseand the windowpartition_clause. - MySQL: walks the sqlparser AST, including
update/delete … order by. It maps each?back to the author's placeholder by line and column. - SQLite: the C serializer now emits a window call's
partitionBy/orderByand an aggregate's ownorderBy.constant_keyswalks the resolved tree. - MySQL oracle: a statement whose only diagnostics are code 14 is checked like any other, not counted as refused.
- Oracle cases, tests, docs: new
param_keys/sqlite_param_keys/mysql_param_keyscases, a mapper test and a loader test. The "A parameter in place of a name" limit is updated on every dialect page. The rebuiltengine.wasmfiles match their sources (tests/fingerprint.test.tspasses).
claude-opus-5-5 ｜ 𝕏
af1408d to
6cf2886
Compare
There was a problem hiding this comment.
✅ No new issues found. Both earlier findings are fixed.
Reviewed changes
Since the last review, the branch was rebased onto main (a5427bf) and now has two commits: a4b51aa (the feature, without its wasm) and 6cf2886. This review covers 6cf2886 and what the rebase changed. git range-diff shows a4b51aa matches the previously reviewed 0234d8c apart from the wasm.
- SQLite position/alias keys: added
Walk::select_keys. It skips a select's ownorder by/group bykey when the select list reads that parameter, because SQLite turns a position or alias into a copy of the result column. Narrowing the skip this way loses nothing: a parameter written twice is alreadyreylo3, so a realorder by :sortnever shares its index with a select-list read. I ran the committedpackages/sqlite/engine.wasmon several cases:order by 2,order by tag,group by 1, 2andorder by tag collate nocaseover a parameter column now give no diagnostics.- A bare
order by :sortand a window'sover (order by :w)are stillreylo14.
- MySQL doc comment: moved
is_variable's doc comment back abovefn is_variable. - Tests and wasm: added position and alias cases to the Postgres, MySQL and SQLite unit tests, and rebuilt all three
engine.wasm.tests/fingerprint.test.tspasses.
ℹ️ PR description still says the docs changed
The rebase dropped the old docs commit (af1408d), because main's 3e697d2 removed the "A parameter in place of a name" limit from the database pages. The PR no longer touches site/, but its description still says that limit "now says the type check refuses such a key" on all three pages. Either edit that paragraph out of the description, or add the guidance back somewhere on the current pages.
claude-opus-5-5 ｜ 𝕏
5767392 to
f0520dd
Compare
There was a problem hiding this comment.
✅ No new issues found. The renumbering to
reylo17is complete and doesn't collide with any other code.
Reviewed changes
Since the last review, the branch was rebased onto main 094287e (#5). That commit took code 14 for duplicate column names, so this PR's diagnostic was renumbered.
CONSTANT_KEY14 → 17: changed the constant inengine/core/src/analysis.rs, the filters in the Postgres, MySQL and SQLite engine tests, everyexpected.txtline inparam_keys,sqlite_param_keys,mysql_param_keysandmysql_types, and the assertions and test name intests/analyze.test.tsandtests/strict-params.test.ts. Noreylo14reference to this diagnostic is left. Codes 13, 14, 15 and 16 are all still in use onmain, and 17 is free. The MySQL oracle's refusal filter readsCONSTANT_KEYrather than a hard-coded number, so it picks up the new code by itself.- Rebuilt artifacts: all three
engine.wasmfiles match their sources atf0520dd(node --test tests/fingerprint.test.ts: 3 pass, 0 skipped). - PR description: the paragraph that still claimed the docs had changed is gone. It now says docs are out of scope, which settles the last review's only note.
claude-opus-5-5 ｜ 𝕏
8d20760 to
dddf278
Compare
dddf278 to
9c2243c
Compare
A parameter is a value, the same for every row, so a parameter that is a whole
order bykey sorts nothing, and one that is a wholegroup bykey folds every row into one group. The analysis accepted both silently, typingsort: string. It is now an error at the parameter, on Postgres, MySQL and SQLite.Before:
select id, name from users order by :sort::texttyped cleanly, and the rows came back in storage order.After: the type check reports, at
:sort:group by :greads "group by :g puts every row in one group, since …" (or "groups every row by the same value" beside another key),partition by :p"puts every row in one partition", and Postgres'sdistinct on (:d)"keeps a single row".What counts as such a key: a bare parameter, or one under casts and collations (
:sort,:sort::text,cast(:sort as text),convert(?, char),$1,?,@s), as a key of a statement'sorder byorgroup by, a window'sorder byorpartition by(inline or a named window), an aggregate's ownorder by(string_agg,group_concat), Postgres'sdistinct on, and MySQL'supdate/delete … order by, in subqueries,withqueries, derived tables and set operations too. A key that holds a parameter stays allowed (case :sort when 'name' then name end,case when :desc then created_at end desc), and so doorder by 2andlimit :n. Grouping sets (rollup(:g)) are not read.on conflicttarget no key matches): the statement is still typed, its row and parameters as before, the error shows in the type check and failsreylo generate, and a build still emits the module.partition by/order byand an aggregate's ownorder by. SQLite already refuses a parameter as a compound'sorder byterm itself.::typewritten on the parameter (:sort::text), as every diagnostic at a cast parameter there does; Postgres's is the placeholder.Verification
param_keys,sqlite_param_keysandmysql_param_keys, blessed on Postgres 18 and MySQL 8.4; every statement there runs over the seeded values.mysql_types/param_order_by.sqlnow records the error.scripts/gates.shcargo-build, engine-tests, oracle-tests, wasm, oracle, typecheck, loader-tests (0 skipped), package and site all pass.A position or alias that SQLite resolves to a result column holding a parameter (
select id, :tag as tag … order by 2) is not refused. Docs are not part of this PR: the "A parameter in place of a name" limit it was going to update has since been cut from the database pages.