Repository navigation
fix(vector): one vector table per notebook with its own embedding width - #47
Merged
Merged
Conversation
Embedding width was hardcoded to 1024 in three places at once: the vec0 DDL (FLOAT[1024]), the `dimensions: 1024` passed to embedBatch(), and the VectorStoreManager default. A vec0 table's width is fixed at creation, so any model that does not return 1024 dimensions could not index at all - the insert fails after the embedding call has already been paid for. That is #33: every embedding model except a 1024-dimensional one fails to index. Vectors now live in one vec0 table per notebook (`vec_<notebookId>`) with the width recorded in `vec_metadata`. That table is the single authority on the width: - The embedding width comes from the model (`embeddings[0].dimensions`) instead of being requested as 1024. - Search, delete and single-document reindex read the width from `vec_metadata`. They no longer pass a guessed default, so they can never create or drop a table based on a wrong number. - A width change is only possible through `rebuildNotebookVectorTable()`, called from the indexing path, which also marks the notebook's documents `pending` so they get re-indexed. A mismatch anywhere else throws. - Existing installs are migrated in `initVectorStore()`: the old global `vec_embeddings` is copied notebook by notebook (`INSERT..SELECT` between vec0 tables) and then dropped. Without this, upgrading users would keep a table the new code never reads. - Deleting a notebook drops its table and metadata row; cascade deletes do not reach vec0 shadow tables. `vectorTableSql.ts` collects every statement that has to interpolate a table name - a SQL identifier cannot be a bind parameter - behind an allowlist, and is covered by `test/vectorTableSql.test.ts`. The packaged smoke test now seeds a legacy 1024-dimensional `vec_embeddings` table, asserts it is migrated and still searchable, and asserts the data-loss case explicitly: a width mismatch must be refused, must leave existing vectors untouched, and only an explicit rebuild may replace them. Design follows #21 by @jimmyken (d37edfa), which introduced per-notebook tables and dynamic widths. That version dropped and recreated a notebook's table whenever a caller asked for a different width, and several call sites (search, reindex) had no width to give, so a single wrong default silently destroyed a notebook's vectors; the migration was also missing. This reworks it so the stored width, not a default, decides. Co-authored-by: jimmyken <four498@gmail.com>
Closed
5 of 9 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 does this PR do?
Moves vectors from one global
vec_embeddingstable (fixedFLOAT[1024]) to onevec0 table per notebook (
vec_<notebookId>), with each notebook's embeddingwidth recorded in
vec_metadata. The width now comes from the embedding modelinstead of being assumed to be 1024.
Why?
A vec0 table's
FLOAT[...]width is fixed at creation. KnowNote asked fordimensions: 1024on everyembedBatch()call and createdvec_embeddingsasFLOAT[1024], so any embedding model that does not return 1024 dimensions failsat insert time - after the embedding call has already been paid for. #33 reports
exactly this: every embedding model except a 1024-dimensional one cannot index.
VectorStoreManageralso carried a mutabledefaultDimensions, so whichevernotebook was indexed last decided the width for the next one.
Related issue
Related to #33 (fixes the dimension half of the report; the "only the first of
several selected PDFs indexes" half is untouched).
Design follows #21 by @jimmyken - see the note at the bottom.
What changed?
src/main/db/index.ts:vec_metadata(notebook_id → table_name, dimensions),plus
getNotebookVectorTable/createNotebookVectorTable/rebuildNotebookVectorTable/dropNotebookVectorTable. Creating a table whosestored width differs from the requested one throws - it never drops rows.
initVectorStore()createsvec_metadataand migrates the old globalvec_embeddings(vec0 → vec0INSERT..SELECT, per notebook, thenDROP),skipping orphan vectors whose notebook no longer exists.
src/main/vectorstore/SQLiteVectorStore.ts: each store resolves its own table.No width → read it from
vec_metadata; no table yet →query/countanswerempty instead of creating a table from a guess.
upsertthrows on a widthmismatch instead of letting sqlite-vec fail cryptically.
src/main/vectorstore/VectorStoreManager.ts:defaultDimensionsandsetDefaultDimensionsare gone; the store, not the manager, knows the width.src/main/services/KnowledgeService.ts: embeddings are requested with themodel's native width; the measured width is fed back via
reconcileVectorDimensions(), which rebuilds the table and marks thenotebook's documents
pendingonly when the width actually changed.src/main/db/queries.ts:deleteNotebook()drops the notebook's vector table;cascade deletes do not reach vec0 shadow tables. Also tightens
createMessage(metadata)fromRecord<string, any>toRecord<string, unknown>, matching the column's own$type<...>().src/main/vectorstore/vectorTableSql.ts(new): every statement that mustinterpolate a table name - a SQL identifier cannot be a bind parameter - behind
a single
safeIdentifier()allowlist.src/main/smokeTest.ts: seeds a legacyvec_embeddings, asserts it migratesand stays searchable, and asserts the data-loss case directly (mismatch is
refused, existing vectors survive, only an explicit rebuild replaces a table).
How was this tested?
npm run typechecknpm test- 20 passing, including the newtest/vectorTableSql.test.ts(identifier allowlist, per-table statements, placeholder counts)
npx eslint/npx prettier --checkon the touched files (one pre-existingno-explicit-anywarning atqueries.ts:200, unchanged)npm run build:unpack && npm run smoke:packaged- PASS, 13 checks, onLinux x64. The output shows the legacy table being migrated, the 768- and
1536-dimensional rebuilds, and the table being dropped with its notebook.
Not tested: a real end-to-end index against a non-1024 provider (OpenAI
text-embedding-3-smallat 1536, Ollamanomic-embed-textat 768). The smoketest covers the storage layer against real sqlite-vec, not a live API call -
worth doing before release.
Screenshots / recordings
Not applicable.
Checklist
npm run typecheckpasses.npm run buildpasses.The
Record<string, any>→unknownline is the one exception (it sits ina file this PR already touches for
deleteNotebook).Desktop / build changes
npm run build:unpackpasses.npm run smoke:packagedpasses.Note on #21
#21 (@jimmyken,
d37edfa) is where this design comes from: one vector table pernotebook, a
vec_metadatatable, widths detected from the model. It could not bemerged as-is - it dropped and recreated a notebook's table whenever a caller
asked for a different width, and the search/reindex call sites had no width to
give, so one wrong default silently destroyed that notebook's vectors; the
vec_embeddingsmigration was also missing (flagged in review on #21). This PRkeeps the design and changes the rule to "the stored width decides, and only the
indexing path may rebuild", plus the migration. @jimmyken is credited on the
commit as co-author; #21 is closed in favour of this.