Repository navigation
fix(engine)!: narrow sqlite strict, postgres written params and call - #186
colinhacks wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Important
A generated column of a STRICT table is now typed as its declared type alone, but SQLite has only enforced STRICT typing on generated columns since 3.51.0. Older SQLite builds still in use (3.45.1 here) store anything in them, so the descriptor is too narrow there.
Reviewed changes
I reviewed the initial commit, which narrows the type descriptor of a SQLite column read as it is when its table is STRICT or when it is a rowid.
Analyzer::exacts: each result column gets anexactflag, set for a bare column reference withstrictorrowid. The flag follows subqueries, views andWITHqueries, and must hold in every arm of a compound. AVALUESarm clears it.descriptor: an exact column with a declared name now gets the single storage class (int64,real,text,bytes) under that name, instead of the affinity union fromstored.- Oracle: there is a new case,
sqlite_strict_columns, and 27sqlite_*cases are reblessed. I checked every added line in those 27 files: each one only narrows anINTEGER,INT,TEXT,REALorBLOBcolumn, as the description says.
The rowid half holds up. columnRef only sets rowid for iColumn<0 or iCol==pTab->iPKey, and iPKey is -1 for INTEGER PRIMARY KEY DESC, for a WITHOUT ROWID key and for INT PRIMARY KEY, which the new case covers. Ordinary STRICT columns and ANY (which stays unknown) also hold up.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The prior finding on generated columns is addressed.
Reviewed changes
I reviewed ed06f5c, which leaves a generated column of a STRICT table out of the exact typing.
exact: now returns(n.strict && !n.generated) || n.rowid.Nodereads thegeneratedflag thatcolumnRefinreylo_sqlite.calready emits, and it defaults to false under#[serde(default)]. A rowid is never generated, so the rowid half is unaffected.- Tests: the engine test adds a virtual generated
gto theSTRICT table and expectsunion.strict_readinsqlite_strict_columnsis reblessed, and onlygandhwiden back to their affinity unions. The comment insetup.sqlis corrected.
ℹ️ The PR description still says generated columns are exact
Under "What holds its declared type alone", the first bullet still reads "generated ones included: SQLite checks what a generated column computes as it stores the row, the virtual kind too". Since ed06f5c that is no longer true. The bullet should move to "What keeps the union" before the squash merge.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues, just one minor suggestion inline.
Reviewed changes
I reviewed e66c91b, which gives a Postgres parameter written to a column that column's type modifier.
RELATION_COLUMNS_QUERY: the query now also readsatttypidandatttypmod, into a newEngine::column_types, in the same round as before. The shadowpg_attributeintests/catalog-queries.test.tsalready has both columns.columns_of: this was split out ofwritten_columns, soParsed::bareresolves to columns the same wayfilleddoes.- Parameter type: a parameter takes
render_typmodonly when every bare column it is written to has the described type and one shared typmod of 0 or more. A cast parameter, one that is only compared, and a column with no modifier (a domain column included) keeprender(o). - Tests: there is a new engine test, and four new
type_modifiersstatements pluscompared.sql.check_unionsandpgvector_typesare reblessed.
Neither the input JSON Schema (engine/core/src/schema.rs) nor .run() reads a Postgres size or precision, so as the description says, nothing changes at run time.
ℹ️ The PR now carries two unrelated changes
The SQLite STRICT change and the Postgres written-parameter change touch separate engines and separate oracle cases. They were batched under a retitled fix(engine): …. AGENTS.md asks for one change per pull request, since each squash-merges as one commit. Splitting e66c91b into its own PR would keep the two changes apart in history and in a revert.
claude-opus-5-5 ｜ 𝕏
There was a problem hiding this comment.
ℹ️ I found no new issues in
88acbdc. The earlier thread onbpchar(=n)for a written parameter is still open and unchanged by this commit.
Reviewed changes
I reviewed 88acbdc, which types the rows of a Postgres CALL as exactly one.
Engine::command: when a statement's first word iscalland the describe returns columns, the cardinality is nowOneinstead ofMany. A statement with no columns still getsNone, and other commands with columns (EXPLAIN,SHOW, …) still getMany. This matches what Postgres does: a procedure withOUTorINOUTparameters sends exactly one tuple, or fails.- Oracle:
commands'call_out.sqlmoves frommanytoone, and nothing else changes. That fits a rule keyed on the statement's kind.
The ! added to the title fits, because .run() now returns the row instead of an array of one. No docs page or skill describes a CALL's rows, so nothing needs updating there.
claude-opus-5-5 ｜ 𝕏
|
On the review's note about two unrelated changes in one pull request: they stay together. Both are places where an engine typed a value wider than the format says, found in one pass, and this repository lets fixes found together share a pull request, a commit each. |
88acbdc to
ed5dd5d
Compare
SQLite checks what a generated column computes only from 3.51, and an app's driver may carry an older SQLite, which stores anything there.
ed5dd5d to
78c9a0f
Compare
Three places where the engines typed something wider than the type descriptor format says they should, each too wide rather than wrong: a SQLite
STRICTcolumn, a Postgres parameter written into a column that has a type modifier, and the rows of a PostgresCALL.A SQLite
STRICTcolumn holds its declared type aloneA column read from a
STRICTtable, and a rowid in any table, now has its declared type alone. They were typed as the union of everything their affinity stores: nothing wrong could pass, but a consumer of the descriptor had to handle values the database refuses to store.What holds its declared type alone
STRICTtable other thanANYor a generated column.INTEGER PRIMARY KEYalias, in any table.WITHquery, or in every arm of a compound.What keeps the union
STRICTtable. SQLite 3.54.1 refuses a computed value of another type, but 3.51.0 stores'1abc'in anintegerone, virtual or stored, and an app may run an older SQLite than the type check.STRICTtable that is no rowid.INTEGER PRIMARY KEY DESC,INT PRIMARY KEYand anINTEGER PRIMARY KEYin aWITHOUT ROWIDtable are no rowids, and each stores'abc'as it is.VALUESincluded.WITH RECURSIVEquery, which is read conservatively.A Postgres parameter written into a column takes its type modifier
Postgres describes a parameter written into a
numeric(10,2)column as plainnumeric, and one written into avarchar(20)column as plainvarchar. A parameter written whole and bare into a column now takes the column's modifier, as the format says: precision and scale, a size, a tick, an interval's fields, an array's for its elements.The server rounds 1.234 written into
numeric(10,2)and refuses a longer string forvarchar(20), so the column's type is the stricter one, never the looser. A parameter under a cast keeps its cast's type, and one only compared with a column keeps the server's. The column query reads each column's type and modifier, one round as before.A Postgres
CALLreturns one rowA procedure with
OUTorINOUTparameters returns their values as exactly one row, and it was typed as any number of rows. Its cardinality is nowone, so.run()returns the row itself rather than an array of one. A procedure with neither still returns no row.Unchanged
The declarations and the docs' captured types for the first two: SQLite's column types are read by their declared names, and a modifier changes no TypeScript type.
.run()changes only for aCALLthat returns a row, above.Tests
sqlite_strict_columns, runs the SQLite rules over seeded edge values: 21 statements, none of them contradicted. 27 othersqlite_*cases'expected.txtchange, 161 lines in all, each a rowid orSTRICTINTEGERcolumn losing its union, or aSTRICTTEXTcolumn losingbytes.type_modifiersgains statements that write parameters by an INSERT, the SELECT that feeds one, an UPDATE and an upsert, cast and bare, and one that only compares them; each runs over the seeded edge values.Verified
cargo build --workspace --all-targetswith no warning; the SQLite engine's tests (44) and the Postgres engine's (202).--strict-coverage: everysqlite_*case and every Postgres case, none failed. Two other Postgres cases'expected.txtgain a written parameter's modifier:check_unions'stierbecomesvarchar(≤10, 'free' | 'pro'), andpgvector_types'sembeddingbecomesvectorof exactly 3.nub run typecheck, andnub run testagainst Postgres 18 and MySQL 8.4 on the release binary: 365 passed, none skipped.nub run captureinsite/writes the same files, so no type on the docs changes.