Say what a read wants, instead of writing out its SQL each time - #1991
Conversation
Every read is the same sentence: these columns, from here, keeping these rows, in this order, at most this many. Each reader wrote that sentence out itself and separately remembered to skip the database when its filter could match no row. A read is now said, not written. `readStatement` turns it into SQL and `readRows` runs it, so the skip is inherited rather than repeated, and `defineReader` builds a collection's reader from its orders plus one function saying what its read is — the listing and attendee readers were the same factory written twice. Two filters join the shared vocabulary: `inSubquery` and `notInSubquery`, which the attendee reader, the group picker and the catalog each had their own version of. Two behaviour changes fall out. An attendee read asking for none of something no longer touches the database, and neither does a transfer read; both used to send a query that could match nothing. `ChosenColumns.queryAll` had no callers left and is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe pull request introduces shared typed SQL read and WHERE-clause builders, then migrates accounting, attendee, listing, chosen-column, and database read paths from manually assembled SQL to structured query objects while preserving filtering, ordering, projection, and null-result behavior. ChangesTyped database read migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
stefan-burke
changed the title
A table already declares its columns, their types, and how to open them. A
read of that table had to say all of it again: the column list as a string,
the row shape as a type, and a separate call to decrypt.
`readerFor(table)` says it once. The filter is the row itself — a value means
"equals", a list means "is one of" — so `one({ slug_index: index })` replaces a
column constant, a row annotation, a filter call and a decrypt call.
Two column lists that were transcriptions of a table's schema are gone. They
could drift from the table they copied and nothing would have noticed: the
site-pages one was a hand-written list of eight columns beside a schema built
from three spread helpers.
Because a filter is now the row's own shape, a wrong column name or a wrong
value type is a compile error rather than a runtime one. It found two things
straight away:
- `getSitePageBySlugIndex` took a plain string where the column is a branded
blind index. Its only caller already passed the right thing.
- The API-key auth read fetched and decrypted the key's name on every API
request, though the auth path never shows it. It now selects the columns it
reads, so the one encrypted column is left alone.
Notes keep their hand-written column list: `system_notes` has no table object
to infer from, so there is nothing there to say twice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
![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: 7ab1424d82
ℹ️ 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".
| const whole = pick(table.columns as unknown as ColumnNames<Row>) as Rows< | ||
| Row, | ||
| Row | ||
| >; |
There was a problem hiding this comment.
When readerFor receives a table with projected fields, this cast makes one and many promise a complete Row even though defineTable deliberately excludes projected fields from table.columns. For example, the rawListingsTable used by the new test omits day_prices and all three image fields, so accessing those required Listing properties can fail despite the returned type claiming they exist. Preserve the physical-row type or require callers to supply the projections instead of asserting this is the whole row.
AGENTS.md reference: AGENTS.md:L132-L137
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 340325c. This was the real one — thank you.
You're right about the mechanism: defineTable sets columns to physicalColumns, filtering out anything projected, so casting that list to the whole row makes one/many promise fields the read never selected. rawListingsTable has four (day_prices, image_alt_text, image_thumb_url, image_url), and my own test used exactly that table — it passed only because it asserted on name.
Rather than preserve a physical-row type (which can't be computed — Table.columns is readonly string[], with no type-level record of which columns are projected), I made the invariant real instead of asserted: readerFor now refuses a table that works any value out from elsewhere, and says to choose the columns with chooseColumns instead. Where the reader exists, its stored columns are the whole row, so the cast is sound.
That's not a loss in practice — every narrow listing read (listingOptionColumns, candidateColumns, catalogListingColumns) already goes through chooseColumns.
Regression test is "refuses a table whose row is more than its stored columns" in test/shared/db/table-reader.test.ts, written before the fix and confirmed failing against the old code:
fail [########################] 11/11 refuses a table whose row is more than its stored columns
FAILED 10 passed, 1 failed
The DB-backed tests moved to attributesTable, which stores its whole row.
Generated by Claude Code
| * rather than the booking. | ||
| */ | ||
| export const attendeeFromWhere = ( | ||
| export const attendeeFrom = ( |
There was a problem hiding this comment.
A repo-wide search finds no importer of attendeeFrom; it is used only by attendees in this same module. Exporting it creates a second public surface for assembling attendee reads without any production caller, so remove export and keep the shared defineReader interface as the supported mechanism.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in 340325c, the export is gone.
Confirmed your search: attendeeFrom is referenced only by attendees in the same module. I created it while splitting the old attendeeFromWhere and carried the export across without checking whether anything still needed it. Nothing did, and a second way to assemble an attendee read is exactly what this change was meant to remove.
Generated by Claude Code
| * keeping these rows, in this order, at most this many. Each reader used to | ||
| * write that sentence out itself, gluing `SELECT`, `FROM`, the WHERE tail and | ||
| * the ORDER BY together by hand, and each one separately remembered to skip the | ||
| * database when its filter could match no row. That is the handwriting this | ||
| * module removes: a reader describes the sentence and this assembles it. |
There was a problem hiding this comment.
This new module comment is largely a comparison with the previous implementation (used to write, handwriting this module removes) rather than information needed to understand the current API. Remove the historical paragraph and retain only the concise current invariant about how a Read is assembled.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 340325c.
You're right that it broke the rule I was supposed to be following: "used to write" and "the handwriting this module removes" describe what the code replaced, which git already records. The header now states only the current contract — what a read is, and that assembling it and skipping the database on an impossible filter happen here.
Generated by Claude Code
| Object.entries(filter).flatMap(([column, value]) => | ||
| Array.isArray(value) | ||
| ? inList(column, value as readonly InValue[]) | ||
| : equals(column, value as Exclude<InValue, null>), |
There was a problem hiding this comment.
When a readerFor filter names a transformed column, these clauses bind the application value directly rather than applying the table column's write transform. For example, filtering apiKeys or newsPostsTable by an encrypted name is accepted by RowFilter but compares plaintext with ciphertext and silently returns no row. Either convert filters through the column definition or reject non-searchable transformed columns loudly instead of exposing a typed operation that produces a false miss.
AGENTS.md reference: AGENTS.md:L105-L110
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 340325c. A genuine hole, and the worst kind — a silent wrong answer rather than an error.
RowFilter accepted { name: "Sealed" } on a table whose name carries a write transform, bound the plaintext, and the database compared it against ciphertext. No match, no complaint, reads as "no such record".
Of the two options you offered I took the second — refuse it. Converting through the column definition would be worse here: an encrypted column's write transform is non-deterministic (a fresh nonce per write), so the converted value would never equal the stored one either. It would look like it worked and still find nothing. A column meant to be searched already carries a separate one-way index column (slug_index, key_index, username_index), which is what a filter should name.
So filterClauses now throws as the call is made, naming the table and column and pointing at the index column. Worth noting it throws synchronously, before any query is built — my first test asserted a rejected promise and failed for that reason, which is how I noticed.
Regression test: "refuses to filter on a column stored in another form" in test/shared/db/table-reader.test.ts, using attributesTable, whose name is encrypted.
No caller was affected — every existing filter names a plain column (id, user_id, key_index, slug_index, sort_order).
Generated by Claude Code
Two ways the reader could give a wrong answer quietly, both now refused. A table whose row holds values it does not store — listings works its images and day prices out from other tables — cannot be read whole, because the columns it stores are not the whole row. The reader promised one anyway. It now refuses such a table and says to choose the columns instead. A filter naming a column stored in another form — an encrypted name — would have compared the plain word against ciphertext, matched nothing, and read as "no such record". It is now refused as the call is made. Also: the module comment for a read described what the code replaced rather than what it does, and `attendeeFrom` was exported with no importer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
…rn-collections-pfvtod
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c339559d6
ℹ️ 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".
| export const inSubquery = bySubquery("IN"); | ||
|
|
||
| /** Keep rows the subquery does NOT name. */ | ||
| export const notInSubquery = bySubquery("NOT IN"); |
There was a problem hiding this comment.
Both new exports are function values whose public signatures are inferred from bySubquery, rather than checked against a declared contract. Give inSubquery and notInSubquery explicit function annotations or define them as exported wrapper functions with explicit return types, so changes to the shared builder fail at the definition instead of silently changing callers.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in 9a93559.
Both were curried out of bySubquery, so their public signatures were whatever the builder happened to infer. They now declare a shared SubqueryFilter contract, so a change to the builder fails where it is defined rather than quietly changing what callers may pass.
Naming the type also made the pair read as what they are: two specialisations of one filter, differing only by keyword.
Generated by Claude Code
| export const listingStatement: (query: GetListingsQuery) => SqlStatement = | ||
| listings.statement; |
There was a problem hiding this comment.
This new export is only another name for listings.statement, so internal callers now see two names for the same mechanism. Export the reader itself and migrate callers to listings.statement rather than retaining this alias.
AGENTS.md reference: AGENTS.md:L79-L79
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in 9a93559. It was a pure alias: listingStatement was assigned straight from the reader's own statement, adding nothing.
The reader is now exported as listingReader, and all seven callers say listingReader.statement(...) — groups.ts, activityLog.ts, listings/attendees.ts, listings/records.ts, and three test files. The alias is gone rather than kept alongside.
I named the export listingReader rather than listings deliberately: listings is the domain word used everywhere for an array of listing records, so an import called listings that is actually a query mechanism would read as the wrong thing at every call site.
getListingRows stays, since it is not an alias — it names the row type the raw read returns (listingReader.rows<ListingRecordRow>), which is the "thin wrapper that adds something" case.
Generated by Claude Code
| export const namedOrder = <Order extends string>( | ||
| orders: Record<Order, string>, | ||
| order: Order | undefined, | ||
| ): string | undefined => (order === undefined ? undefined : orders[order]); |
There was a problem hiding this comment.
A repo-wide search finds that namedOrder has no production importer: its only production use is inside defineReader in this module, while the only external import is its direct unit test. Remove the export and test the behavior through defineReader instead of adding a public surface solely for tests.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — fixed in 9a93559. namedOrder is private again.
Confirmed your search: its only production use was inside defineReader in the same module, and its only import was its own unit test. It had been exported for the two readers to call before defineReader existed, and I left the export behind when they stopped needing it.
The order behaviour is still covered, now through the mechanism production actually uses: defineReader is built with two named orders in test/shared/db/read.test.ts, and the tests assert the generated SQL carries the right ORDER BY for each, and none at all when no order is asked for. That is a stronger test than the old one — it pins the clause as it reaches the query rather than the lookup in isolation.
Generated by Claude Code
`listingStatement` was only another name for the reader's own `statement`, so callers saw two names for one mechanism. The reader itself is now exported and its seven callers say `listingReader.statement(...)`. `namedOrder` had no caller outside its own module — only its test — so it is private again, and the order behaviour is tested through `defineReader`, which is what production uses. The two subquery filters carried inferred signatures; they now declare one, so a change to the shared builder fails where it is defined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UASQuuj6grCBBrHHQeqMCF
2 participants
What changed
Two steps, in order.
1. A read is said, not written
Every read in the codebase is the same sentence: these columns, from here,
keeping these rows, in this order, at most this many. Each reader used to
write that sentence out itself — gluing
SELECT,FROM, the filter tail, theORDER BYand theLIMITtogether by hand — and each one separately rememberedto skip the database when its filter could match no row.
One place now turns a said read into SQL, and one place runs it.
the attendee reader each had their own copy of "build the columns, build the
tables, look up the named order, collect the filters, run it". They now share
one factory: a collection's reader is its list of orders plus one function
saying what its read is.
already answered, so it should cost no trip to the database.
"rows another query does not name" each existed in three private versions.
2. A table read says which rows it wants, and nothing else
A table already declares its columns, their types, and how to open them. A read
of that table still had to say all of it again: the column list as a string, the
row shape as a type, and a separate call to decrypt.
Now it says it once:
The filter is the row — a value means "equals this", a list means "is one of
these" — so one item and many are the same filter, never two paths.
Two column lists that were transcriptions of a table's schema are gone. They
could drift from the table they copied and nothing would have noticed: the
site-pages one was a hand-written list of eight columns sitting beside a schema
assembled from three spread helpers, so you could not tell by reading whether it
was still right.
Two fixes the types found
Because a filter is now the row's own shape, a wrong column name or a wrong value
type is a build error rather than a runtime one. It found these immediately:
never shows the name — the comment above it said so — but the hand-written
column list included it anyway. The read now names the columns it uses, so the
one encrypted column is left alone.
Its only caller already passed the right thing, so the parameter was simply too
loose.
Two more from the first step, both cases of asking for nothing and being charged
anyway: an attendee read given an empty list of ids, and a transfer read,
each used to send a query that could not match a row.
What is deliberately left alone
their answers, news cards with their image, images with their sort order. No
table describes those shapes, so nothing can infer them. They say their read
the ordinary way, which is the honest boundary: one table and its own columns
infers, anything else does not.
system_noteshas no table object at all, so its column list is nota copy of anything and there is nothing to unify.
a genuine inconsistency elsewhere (
built-sitesexposes snake_case columnnames on a camelCase row). That is a real thing to fix, but not here.
About size
This does not make the codebase smaller, and that is worth saying plainly.
The first step alone measured +88 lines and +275 syntax nodes across
src/; thesecond adds a 95-line module and removes 62 lines from the files it touched.
The reason is structural rather than something more migrations would fix: a piece
of SQL written as text is one leaf of the syntax tree however long it is, while
the same read described as data is a small tree of its own. Two conversions were
tried purely for terseness, measured, and reverted for adding size without
removing anything.
What it does remove is knowledge written down twice — two column lists that
could silently drift from their tables, a reader factory written twice, three
private copies of the same two filters, and eighteen column names that nothing
was checking. That is the case for this change; brevity is not.
Tests
values that fill them (including a join carrying its own value), and that a
read matching nothing still produces valid SQL for a caller embedding it in a
batch.
a value filters by equals and a list by is-one-of, several columns must all
match, a narrowed read can still filter on a column it does not select.
exact-SQL assertions are unchanged.
Full checks pass, including complete test coverage.
Generated by Claude Code