`join()` shipped with zero cases in the generic tabular suite — the only shared block asserts which path ran, and the pushdown/fallback disagreement it missed shipped one day later

[sroussey]

What

5f83ae85f (feat(storage)!: add join() to ITabularStorage with SQL pushdown, #893) made join a required member of ITabularStorage (ITabularStorage.ts:796, no ?) with two implementations of the same semantics: the SQL pushdown in BaseSqlTabularStorage.runSqlJoin, and the hash fallback in BaseTabularStorage.join that every other backend inherits.

The shared contract got one block for it, and it asserts path selection, not results:

$ grep -n 'it(\|itImpl(' packages/test/src/contract/tabular-storage/assertions/joinPushdown.ts
61:      itImpl(   "runs a same-connection join without querying the right side"
78:      itImpl(   "falls back to the hash join for a right side on another connection"

Two cases. Both use the same spec — type: "left", one on pair, one orderBy, no where, no limit, no offset, no compound key, no null keys — and both assert rightQuery was/was not called plus a two-row shape.

The generic suite that every backend rides has nothing:

$ grep -c 'join' packages/test/src/test/storage-tabular/genericTabularStorageTests.ts
0

And joinPushdown is gated on createSiblingStorage, which only three backends supply:

$ grep -rl 'createSiblingStorage' packages/test/src/test/storage-tabular/
packages/test/src/test/storage-tabular/DuckDbTabularStorage.integration.test.ts
packages/test/src/test/storage-tabular/PostgresTabularStorage.integration.test.ts
packages/test/src/test/storage-tabular/SqliteTabularStorage.integration.test.ts

$ grep -rl 'runTabularStorageContract' packages/test/src/test/storage-tabular/ | wc -l
10

So seven of the ten backends that ride the tabular contract get no join assertion of any kind, and all seven use the hash fallback: InMemory, SharedInMemory, IndexedDb, Cached, Telemetry, FsFolder, Supabase.

Why it matters

The bug this predicts already happened, one day after the feature landed. 701589c30 (#934):

planSqlJoin sent a join to the hash fallback when the right-hand storage sat inside a connection transaction the left side was not enlisted in, by testing right.inTransaction. … on a real pg.Pool the guard was dead: the pushdown ran, read the right-hand table off a different pooled client, and missed the transaction's uncommitted rows. An inner join returned zero rows where the same call on SQLite, or through the hash fallback, returned the row — a wrong answer with no error, differing between backends and between the two strategies for identical inputs.

That is precisely the property the contract does not state. joinPushdown would have stayed green through it: its own fixture is not inside a transaction, and it never compares the two strategies' rows for one spec.

The interface docstring lists the semantics that differ and are unasserted anywhere shared (ITabularStorage.ts:409-796): where.right "applied as part of the join condition rather than after it, so under a left join an unmatched left row survives"; limit/offset "applied to the joined rows"; the hash join "bounds the left read — and stops early — only when the joined order does not need a sort"; "A join key that is null (or absent) on either side never matches, as in SQL." Four rules with two implementations each and no shared case.

Per-backend coverage exists and is good (45 co-located cases across JoinSql.test.ts 11, hashJoin.test.ts 27, joinDelegate.test.ts 7, plus SQLite/Postgres/Scoped/Cached integration cases) — but it tests each half in isolation. Nothing runs the same spec down both paths and compares.

Proposed fix

1. Add a joinSemanticsBlock to runTabularStorageContract (so all ten backends run it), covering the four documented rules above: null key never matches, where.right under a left join, compound on, limit/offset applied to joined rows, orderBy across sides.
2. Add the parity case the fallback/pushdown split needs and joinPushdown does not have: on a backend that supports both, run one non-trivial spec through the pushdown and through the fallback (same rows, sibling storage vs a deliberately foreign right side) and assert equal results, not just which path ran. guardParity.ts in the same directory is the template — it compares outcomes across two routes over a derived method set rather than listing them.
3. InMemoryTabularStorage is the free reference implementation for the semantics half, the same way it is for the new vector contract (The new IVectorStorage contract runs on 6 of 8 implementations — the two left out are the one the KnowledgeBase uses on SQLite and the Supabase backend #941).

Measured on origin/main @ 2d36880 (0.6.0). bun scripts/test.ts vitest unit storage → 59 passed / 2 skipped of 63 files, 1176 tests (the two failures here are wall-clock timeouts on PGlite boot in a slow sandbox, not assertions).

Found during the 2026-09-14 review of packages/.

[sroussey]

Landed on claude/keen-dijkstra-i4bne2 as fb4f53b.

One correction to the premise first

The grep -c 'join' genericTabularStorageTests.ts → 0 is accurate but reads the wrong file. packages/test/src/test/storage-tabular/genericTabularJoinTests.ts has existed since 5f83ae8 — the same commit that added the feature — with 16 shared cases and its own two-table fixture, wired into nine backends:

$ grep -rl 'runGenericTabularJoinTests' packages/test/src/test/storage-tabular/*.test.ts | wc -l
9   # InMemory, IndexedDb, Cached, Telemetry, DuckDb, HttpProxy, Supabase, Postgres, Sqlite

So "seven of the ten backends get no join assertion of any kind" is not the case, and most of proposed fix (1) was already there: null keys never matching, where.right under a left join, compound on, limit/offset on the joined rows, and ordering across sides with nulls first under ASC and last under DESC all have cases. Writing joinSemanticsBlock would have duplicated them.

What was genuinely missing is fix (2) — and two more things.

What landed

join strategy parity. Runs the same spec through the pushdown and through the inherited hash join, on the same two instances (BaseTabularStorage.prototype.join.call, so same data, same driver, one difference), and asserts they agree. Specs are derived rather than listed, as guardParity.ts does: a mapped type over Required<JoinSpec> means a field added to JoinSpec fails to compile until it declares what the two strategies owe each other, crossed with the axes that actually interact — orderBy against limit/offset.

join bounded left read. The rule only the fallback can break: it bounds the left read and stops early only when the joined rows are already in their final order. Losing the bound puts a whole remote table on the wire for a limit: 2; applying it too eagerly returns the first N rows of an arbitrary order as though they were the top N. Neither was asserted anywhere.

SharedInMemory now rides the join contract — every other in-memory backend already did. FsFolder stays out and keeps its existing case asserting join throws StorageUnsupportedError, which is its contract.

On the comparison, and where it is weaker than it looks

Holding the two strategies to an identical row order is wrong, and the block caught me doing it on the first run. join promises orderBy, limit and offset apply to the joined rows — it never promises what order an unordered join arrives in. SQLite and Postgres happen to return a stable one; DuckDB, columnar and parallel, does not. So an unordered join is compared as a set, and an unordered windowed one by row count and membership, which is the whole of what is determined. Anything stricter asserts the storage engine's row layout and fails a backend that is behaving correctly.

Fail-first evidence

Reinstating #934's right.inTransaction guard does not turn the parity block red, and I do not think it can: under the pooledFacade over PGlite that PostgresPooledJoinTransaction.test.ts uses, both strategies read the same session, so the rows agree whichever one runs. That file already covers #934 by asserting the strategy choice, and still does. Row parity would only catch it against a real pool.

So I proved the blocks by mutation instead, both restored afterwards:

Unbinding the hash join's early-stop condition (boundLeftRead = needed !== undefined, dropping the pushLeftOrder || orderBy.length === 0 guard) fails 10 parity specs and 2 bounded-read cases across SQLite, Postgres and DuckDB — and not one of the 16 pre-existing cases, because none of them pairs a bound with an order that needs sorting:

reads the whole left side when the joined order needs a sort
reads the whole left side when the order names a nullable left column
the pushdown and the hash join agree on left join, a right order, a limit of 2, no offset
the pushdown and the hash join agree on left join, a nullable left order, a limit of 2, an offset of 1
... 8 more

Flipping the SQL join's NULLS FIRST/NULLS LAST fails the parity block on all three SQL backends, including the nullable-left-column orderings nothing ran before.

Verification

bun run build, bun run format-check, bunx oxlint --type-aware --deny-warnings all clean. bun scripts/test.ts vitest unit storage → 61 passed / 2 skipped of 63 files, 1216 tests (baseline on this branch point was 1178; both PGlite files passed here, so the sandbox timeouts you saw did not reproduce). vitest integration storage → 26 files, 1968 tests, 0 failures. Full vitest unit → 691 files, 8624 tests; bun unit → 683 files, 8540 tests.

The suite moved to @workglow/test-contract/tabular-storage as runTabularJoinContract — see #945.

---

Generated by Claude Code