docs: take over #678's embedding-API spec, reconciled with the tree (supersedes #678) - #693
Open
dpsiderius wants to merge 5 commits into
Open
docs: take over #678's embedding-API spec, reconciled with the tree (supersedes #678)#693dpsiderius wants to merge 5 commits into
dpsiderius wants to merge 5 commits into
Conversation
…(013/Req-7) 010 gains Requirement 8: a write MUST NOT succeed while leaving an index it could not read unmaintained. Measured at 0.18.5 against stock sqlite3 3.51.0 -- inserting into a stock-created composite-PK table leaves rows out of sqlite_autoindex_*, after which the oracle undercounts and integrity_check reports rows missing, while the write returns rc=0. Names both acceptable fixes (recover the autoindex column list from the declared constraint, or refuse the write) and scopes autoindex *creation* out to V3/V7. The embedding-api spec gains Requirement 7: statements must yield rows incrementally. It also upgrades the composite-PK prerequisite from inferred to measured, flagging it as the highest-priority item in or around that spec -- it is silent corruption of a valid SQLite file, not an ergonomic gap, and its Req 6 byte-identity scenario cannot pass while it stands. That spec is numbered 012 as of this commit and is renumbered to 013 by the next one, after main landed its own spec 012. Refs: 010/Req-8, 013/Req-7
Main landed its own spec 012 (`012-query-constraints`) and its own ADR-0033 (constant propagation / OR-to-IN) while this branch was open, so both numbers collided on rebase. The newcomer moves: - `.openspec/specs/012-embedding-api/` -> `013-embedding-api/`, heading updated - `adr/0033-embedding-api-owns-the-connection-driver-out-of-tree.md` -> `0034-`, heading updated, and its five `spec 012` prose references retargeted to 013 - `adr/index.md` gains the 0034 row, which 0ee936b omitted entirely Renumbering ADR-0034 does not violate the immutability convention: it has never been on main and is still `Status: Proposed`. Also fixes four `**Implementation:**` lines in spec 013 that were already marked `(planned)` but written in a form `tools/assurance.py:474` does not match -- its regex only accepts `(planned)` immediately after a *single* backticked path, so `` `a`, `b` (planned) `` scored as active and its not-yet-written test links counted as dead. With that fixed, the dashboard is byte-identical to main: 86 active requirements, Completeness 85/86 (99%), Coverage 270/270 (99%), zero dead links, planned 2 -> 10. Refs: 013/Req-1, 013/Req-7
…ith the tree (#678) Takes over #678's spec so it can land off current `main`. Jacob's three commits are cherry-picked verbatim above this one and keep their authorship; everything here is the reconciliation they could not carry, because the tree moved after they were written. ADR renumber, 0034 -> 0041. 0034 was already taken on `main` (`0034-index-range-seeks.md`) when #678 was opened, and 0038/0039/0040 have since gone to the Cargo registry decision (#684), `Value`'s `Arc` payloads (#688) and the streaming primitive (#683). Three citations in spec 013 and the `index.md` row move with it. The index gains a gap at 0039/0040 until those two branches land, which is the correct state for this branch rather than a placeholder. Four claims in spec 013 were true when written and are not now: - **Item 7 said "nothing in `src/vdbe/` offers a step or iterator API".** `vdbe::Execution` is that API as of #683, with `run()` reimplemented as a wrapper over it. Requirement 7 is restated as a facade gap and its `Implementation:` line now points at `Execution::next_row` as the thing to build on -- #682 found the ordering matters, because a facade retrofitted onto `execute_with_db` cannot be made incremental afterwards. - **Item 3 attributed the `!Send` problem to the pager alone.** So did ADR-0041. `Value::Text`/`Blob` held `Rc` payloads, which made a result row -- precisely the thing that has to leave a worker thread -- unsendable too. Closed by #688/ADR-0039, which also narrows Requirement 4: the worker thread is still required for the pager, but it now hands rows across instead of copying them. - **The composite-PK prerequisite was one item; it is two.** #685 fixed maintenance of an existing `sqlite_autoindex_*` and the read-only safety valve (spec 010/Req 8). Creating one on `CREATE TABLE` is still open as #687. SQE's two workarounds split the same way: the write-refusal one comes out now, the dropped-composite-key one waits on #687. - **Requirement 7's acceptance scenario could not pass.** It asked that "peak allocation is proportional to the ten rows". Peak heap for a streaming read is a floor set by the page cache, not a slope in rows pulled, so no correct implementation satisfies that wording. Restated as independence from result size -- 8.68 MB flat against 137.7 MB materialized on 1,000,000 rows (#682) -- with `DEFAULT_PAGE_CACHE_CAPACITY` named as the knob that moves the floor (2000 pages -> 8.68 MB, 64 -> 291 KB, ~4.5% streaming cost). That is both the property a consumer needs and, unlike proportionality, testable. Also corrected in both specs: measurements were cited against "stock `sqlite3` 3.51.0", but the pinned oracle is 3.53.4 (`tests/corpus/oracle.rs:22`, `Cargo.toml [package.metadata.oracle]`). On this machine a bare `sqlite3` is Apple's 3.51.0 codec build, which `tools/gen_fixtures.sh` refuses by design. I re-derived the autoindex rule against the pinned 3.53.4 across eleven DDL shapes, comparing `pragma_index_info` key lists rather than index counts: every case agrees, including the counter-intuitive ones (declaration order beats primary-key-first; a rowid alias consumes no number). The citation was wrong, not the rule -- but "measured against the pinned oracle" is this repo's whole assurance basis, so it has to be accurate. Spec 010/Req 8's three scenario `Tests:` links already name the exact test functions #685 created. They stay `(planned)` here: the flip belongs to the PR that discharges the requirement, and it cannot happen on this branch because the tests do not exist on it. `make check-assurance` passes at 86/86 and 276/276, unchanged -- every requirement added here is `(planned)`, so it is excluded from scoring by design and the dashboard cannot move until the implementing tickets land. No dead links. Refs: 010/Req-8, 013/Req-1, 013/Req-4, 013/Req-7, #678, #682, #683, #685, #687, #688 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in spec 013 (#678) My reconciliation commit said four of spec 013's claims had stopped being true. There were five. Item 6 read "`Pager` syncs but nothing states what is guaranteed, and `synchronous` has no handler". The second clause was already false when #678 was written: #645 implemented `PRAGMA synchronous` in full — the bare query form reporting `0`/`1`/`2` like stock SQLite, all three levels, and a decided per-level fsync-skip policy recorded in ADR-0036 (`src/vdbe/pragma.rs:79`, dispatched at `src/vdbe/exec.rs:760`, with unit tests). Nothing about it is a stub. That also means Requirement 5 is further along than it claims. It asks the API to "honor `PRAGMA synchronous` at least to distinguish FULL from OFF", which is a weaker ask than what already exists, and says "what is missing is a documented guarantee and any way to trade it" — the trade mechanism is exactly what #645 added. Requirement 5's remaining work is the written guarantee, the transaction surface, and the busy/retryable error handling, not the PRAGMA. I found this while answering "will this work with SQE yet", by checking each of the spec's seven gaps against the tree instead of trusting the list. Worth noting for anyone reviewing #693: the list was written against 0.18.5 and the tree is 0.18.10, so treat every "is missing" line as a claim to re-verify rather than a fact. The four I corrected first were the ones my own branches falsified; this one had been stale for longer and nothing I built touched it. `make check-assurance` unchanged at 86/86 and 276/276, no dead links — Requirement 5 stays `(planned)`, since the requirement as a whole is not discharged even though this part of it is. Refs: 013/Req-5, #645, #678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
#678's spec, landed off current
main. This supersedes #678 — close thatone when this merges; do not merge both.
Jacob's three commits are cherry-picked verbatim (same content, same
messages) and keep their authorship: GitHub shows
Jacob Verhoeksas author, me as committer, and he lands in thecontributor list. Everything else is one reconciliation commit of mine.
Why a takeover rather than pushing to #678
#678 was opened from a fork point well behind
mainand its spec makesseveral claims that were true when written and are not now. Rather than ask
for a rebase and a round of edits on someone else's branch, this takes the
content over and states the corrections in a commit with my name on it — so a
reviewer can see exactly what I changed and disagree with any of it
separately from Jacob's original work.
Why this should merge before the other four
Spec 013 and spec 010/Requirement 8 exist only on this branch. #689
(#685), #690 (#688), #691 (#683) and the rows-changed PR all carry
Refs:lines naming
010/Req-8,013/Req-1,013/Req-4and013/Req-7. Untilthis lands, those references point at requirements the tree does not contain,
and #689's seven new corpus tests cannot get the per-scenario
**Tests:**links this repo's traceability convention asks for.
Nothing else about the ordering is load-bearing. ADR numbers only need to be
unique, so the 0039/0040 gap this leaves in
index.mdcloses itself when#690 and #691 land.
The ADR is renumbered 0034 → 0041
0034 was already taken by
0034-index-range-seeks.mdwhen #678 was opened,and 0038/0039/0040 have since gone to the Cargo registry decision (#684),
Value'sArcpayloads (#690) and the streaming primitive (#691). Threecitations inside spec 013 and the
index.mdrow move with it.Five claims that stopped being true
src/vdbe/offers a step or iterator API"vdbe::Executionis that API (#691), withrun()reimplemented as a wrapper over it!Sendis the pager's faultValue::Text/BlobheldRc, so a result row — the one thing that must leave a worker thread — was unsendable too (#690)CREATE TABLEis still open (#687)synchronoushas no handler"The
synchronousone is the one to take as a warning: four of these five werefalsified by my own branches, but that one had been stale since before #678
was opened, and I only caught it while checking whether SQE can use this yet.
The list was written against 0.18.5 and the tree is 0.18.10 — treat every
"is missing" line in this spec as a claim to re-verify, not a fact.
The
Sendone is worth a reviewer's attention: both this spec andADR-0041 attribute the problem to the pager alone, and neither mentions
Value. The pager half is still real and is still what Requirement 4'sworker thread exists for — but the
Valuehalf was the part that made aSend + Synchandle impossible, and nothing in the original spec saw it.Requirement 7's acceptance scenario could not pass
It asked that "peak allocation is proportional to the ten rows". No
correct implementation satisfies that wording, because peak heap for a
streaming read is a floor set by the page cache, not a slope in rows
pulled. Measured on 1,000,000 rows (spike 014, #682): 137.7 MB materialized
against 8.68 MB streamed, and the 8.68 MB does not move with result size.
Restated as independence from result size, which is both the property a
consumer needs and — unlike proportionality — testable. The whole floor turns
out to be one constant,
DEFAULT_PAGE_CACHE_CAPACITY(src/pager.rs:63):2000 pages → 8.68 MB, 256 → 1.10 MB, 64 → 291 KB, at ~4.5% streaming
throughput for the smallest. So a caller who needs the floor lower has a
knob, and the scenario now describes something that can be asserted.
Both specs cited the wrong oracle
Measurements were attributed to "stock
sqlite33.51.0". The pinned oracleis 3.53.4 (
tests/corpus/oracle.rs:22,Cargo.toml [package.metadata.oracle]). On macOS a baresqlite3is Apple's 3.51.0codec build, which
tools/gen_fixtures.shrefuses by design.I re-derived the autoindex numbering rule against the pinned 3.53.4 across
eleven DDL shapes, comparing
pragma_index_infokey lists rather than indexcounts. Every case agrees, including the counter-intuitive ones —
declaration order beats primary-key-first, and a rowid alias consumes no
number. So the citation was wrong, not the rule. Fixing it anyway, because
"measured against the pinned oracle" is this repo's entire assurance basis
and it has to mean what it says.
What is deliberately left alone
Spec 010/Req 8's three scenario
**Tests:**links already name the exact testfunctions #689 created — Jacob predicted the file and function names
correctly. They stay
(planned)here: the flip belongs to the PR thatdischarges the requirement, and it cannot happen on this branch because those
tests do not exist on it. #689 should get a follow-up commit dropping the
three markers once this lands.
Test plan
make check-assurance— 86/86 and 276/276, unchanged, no deadlinks. A spec-only PR moving nothing is the correct outcome here:
every requirement added is
(planned), whichtools/assurance.pyexcludes from all scoring by design. The dashboard moves when the
implementing tickets land, not when the promise does
make lint(both clippy passes) andmake check-mod-filesclean.openspec/onlyauthor=Jacob Verhoeks <jjverhoeks@schubergphilis.com>Three design questions for Jacob, not blocking
These are genuine choices the spec makes that I did not want to silently
overrule, and they belong in conversation rather than in my commit:
Send(feat: make Value Send by switching Text/Blob payloads to Arc (#688) #690), is onethread per connection still the shape you want, or does the pager's
Rcgraph want revisiting on its own terms?
at ~4.5 µs/row and chunking at ~1024 erasing it. Adaptive chunking
(1, 2, 4 … 1024) came within 5% of the best fixed size on drain and 10% on
first-row latency, with no knob. Should the facade do that, or expose the
chunk size?
DEFAULT_PAGE_CACHE_CAPACITYbecomeper-connection configuration, given it is the entire streaming memory
floor?
Spend: small — the cherry-picks are mechanical; the cost was reading the
spec against the tree closely enough to find the four stale claims and the
unsatisfiable scenario.
Refs: 010/Req-8, 013/Req-1, 013/Req-4, 013/Req-7, #682, #685, #687
Supersedes #678
🤖 Generated with Claude Code