Skip to content

perf(spac): one indexed pass over s1_classification; default sec resolve's version - #297

Open
sroussey wants to merge 1 commit into
mainfrom
claude/keen-knuth-d74u5n-scan-and-resolve
Open

perf(spac): one indexed pass over s1_classification; default sec resolve's version#297
sroussey wants to merge 1 commit into
mainfrom
claude/keen-knuth-d74u5n-scan-and-resolve

Conversation

@sroussey

Copy link
Copy Markdown
Contributor

Two independent operability fixes.

(a) spacCandidateScan: two correlated subqueries over an unindexed table

buildScanSql read s1_classificationtwice per candidate entity — a SELECT-list correlated scalar subquery (filed_sic_6770) and a WHERE EXISTS (filedSicMatch) — against a table whose only index is its (extractor_id, accession_number) primary key. Neither could use it.

Both fixes land, because the index alone does not remove the per-row work:

  1. Index.indexes: [["cik", "sic"]] on the S1_CLASSIFICATION_REPOSITORY_TOKEN entry in storageRegistry.ts. It reaches existing databases with no migration: setupDatabase() calls createTableAndIndexes() on every run, emitting CREATE INDEX IF NOT EXISTS on both backends, and the PK-prefix redundancy check does not suppress it (cikextractor_id).
  2. Restructure. Both fragments collapse into one pre-aggregated LEFT JOIN:
LEFT JOIN (SELECT cik, max(CASE WHEN sic = ? THEN 1 ELSE 0 END) AS filed_sic_6770
FROM s1_classification WHERE sic IS NOT NULLGROUP BY cik) c
ONc.cik=e.cik

with c.filed_sic_6770 = 1 in the WHERE. Semantics preserved exactly: no matching group yields NULL from the LEFT JOIN, which is the documented "not asked yet" — different from false.

⚠️ This surfaced a real pre-existing SQLite bug

The parameter-order trap is not hypothetical; main already trips it.bind() appends in declaration order but SQLite numbers ? by position in the statement, and on main:

  • filedSic6770 is declared third in the SELECT-list block but appears fourth in the emitted SELECT list (after renamedFrom); and
  • sinceClause is declared beforefiledSicMatch but appears after it.

So every parameter from that point on is shifted by one. Concretely, on SQLite filed_sic_6770 came back false for a filer whose registration really was filed under a 6770 header — the one signal a completed de-SPAC cannot erase, and the whole reason that column exists. Postgres was unaffected ($n is numbered by bind order), so the two backends disagreed silently.

This PR fixes it as a side effect: BLANK_CHECK_SIC now binds in the FROM clause and filedSicMatch binds nothing, so declaration order and statement order line up (SELECT-list subqueries → derived-table sic → WHERE predicates). The buildScanSql JSDoc now spells the rule out and notes that a slip yields a silently wrong scan rather than an error, on one backend only.

Tests

The existing SQL-vs-scanRepository parity assertions are the correctness guard for the rewrite. The seed grows three CIKs that reach the s1_classification fragments:

  • 8 — a completed de-SPAC (Joby Aviation, Inc., SIC 3721) whose only signal is a 6770 s1_classification row;
  • 9 — a parsed registration whose header carried no SIC, which must stay filed_sic_6770: null, never false;
  • 10 — several parsed registrations, one of them 6770.

CIKs 8 and 10 are what failed against main, with the repository twin reporting true and the SQL reporting false — the binding shift, caught by exactly the assertion that exists to catch it.

A new plan assertion pins the access shape:

constreads=plan.filter((r)=>/\bs1_classification\b/.test(r.detail));expect(reads.map((r)=>r.detail)).toHaveLength(1);expect(reads[0].detail).toMatch(/COVERINGINDEX/);expect(plan.find((r)=>r.id===reads[0].parent)?.detail).toMatch(/^MATERIALIZE/);

It asserts the count rather than the absence of the literal string SCAN s1_classification, because the correct plan legitimately reads SCAN s1_classification USING COVERING INDEX s1_classification_cik_sic under MATERIALIZE c — one pass for the whole scan. The two correlated fragments show up as two rows, so this fails both against current code and against a version that adds the index but keeps the EXISTS correlated (which reads as two SEARCH ... USING COVERING INDEX (cik=?) rows). buildScanSql is exported for the assertion.

(b) sec resolve --kind company --all --renormalize could not run

--resolver-version was a .requiredOption (resolve.ts:24), so commander rejected the documented re-key ceremony's step 3b — the one step CLAUDE.md marks REQUIRED and silent-if-skipped.

The docs are right about intent, and asking an operator to look up a semver mid-ceremony is exactly the failure mode the doc warns about, so the flag is defaulted rather than the docs changed:

  • .requiredOption.option("--resolver-version <semver>", "target resolver semver (default: the active slot)"), typed resolverVersion?: string.
  • When undefined, resolved after the --kind / batch-resolvable checks via getActiveSlot(new VersionRegistry(globalServiceRegistry.get(COMPONENT_VERSION_REPOSITORY_TOKEN)), "resolver", kind) — "next if a dev cycle exists, else current", the same rule ResolverCoverageTask and RoleQuery already read. Absent, it throws in ComputeFormsWorklistTask's wording: No active slot for resolver '<kind>'. Run 'sec db setup' to bootstrap.
  • isValidSemver validation is kept for an explicitly supplied value; a registry-sourced one was already validated when written.
  • The resolved version is echoed in the existing summary line, so the operator sees which slot ran.

Tests

resolve.test.ts gains a case running the documented command verbatim with no--resolver-version through runCliProcess, asserting exit 0 and that stdout names the bootstrapped slot (resolved 0 company observation(s) at v1.0.0). Confirmed failing first against main with commander's required-option error and exit 1. A sibling case asserts an explicit --resolver-version 2.3.4 still wins; the three existing malformed-semver cases are unchanged and still error.

Verification

bunx vitest run src/task/spac/ src/config/
Test Files 24 passed | 1 skipped (25)
bunx vitest run src/cli/groups/resolve.test.ts
Test Files 1 passed (1)
Tests 12 passed (12)
bunx vitest run src/cli/groups/version.test.ts
Test Files 1 passed (1)
Tests 12 passed (12)
bun run build # bun build + tsc, clean

Note: version.test.ts spawns CLI subprocesses with 15 s per-case timeouts and times out when run concurrently with the other CLI suites on this machine. It passes when run alone, before and after this change — a test-harness load artifact, not a regression.

Docs

CLAUDE.md's re-key ceremony notes that step 3b takes no --resolver-version and defaults to the active slot.


Generated by Claude Code

…s version
Two independent operability fixes.
**Scan.** `buildScanSql` read `s1_classification` twice per candidate entity —
a SELECT-list correlated scalar subquery and a WHERE `EXISTS` — against a table
whose only index is its `(extractor_id, accession_number)` primary key. Both
fragments collapse into one pre-aggregated `LEFT JOIN`, and the registry gains
an `(cik, sic)` index; `setupDatabase()` re-emits `CREATE INDEX IF NOT EXISTS`
every run, so existing databases pick it up with no migration. Semantics are
unchanged: no matching group yields NULL from the LEFT JOIN, which is the
documented "not asked yet" rather than false.
Moving the `BLANK_CHECK_SIC` bind out of the SELECT list surfaced a real
**pre-existing SQLite bug**: `bind()` appends in declaration order but SQLite
numbers `?` by POSITION, and `filedSic6770` was declared third while appearing
fourth (and `sinceClause` was declared before `filedSicMatch` while appearing
after). Every parameter from there on was shifted by one, so on SQLite
`filed_sic_6770` came back false for a filer whose registration really was
filed under a 6770 header — the one signal a completed de-SPAC cannot erase.
Postgres was unaffected (`$n` is numbered by bind order), so the two backends
disagreed. The new fixtures make the existing SQL-vs-repository-twin parity
assertions catch it, and a plan assertion pins the table to one materialized,
index-covered read.
**Resolve.** `--resolver-version` was a `requiredOption`, so commander rejected
`sec resolve --kind company --all --renormalize` — the re-key ceremony's step
3b, the one step the docs mark REQUIRED and silent-if-skipped. It now defaults
to the active slot ("next if a dev cycle exists, else current"), the same rule
`ResolverCoverageTask` and the role query read, and the resolved version is
echoed in the summary line. An explicitly supplied value still wins and is
still semver-validated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@sroussey@claude