Conversation
) Add pgvector HNSW indexes with cosine distance on embeddings_text (768-d), embeddings_visual (512-d), and embeddings_face (512-d) so vector queries use index scans instead of sequential scans. - Migration 0024 creates the three indexes using vector_cosine_ops - Update deferral comments in 0001_init.sql and 0019_versioned_index_generations.sql - Update face.go package comment to reference the new index Fixes bytefolk#173
Add verification script and validation ledger documenting the EXPLAIN regression test methodology for HNSW ANN indexes. - scripts/verify_hnsw_indexes.sh: automated EXPLAIN test - docs/VALIDATION_HNSW.md: test methodology, expected results, failure modes Actual database execution pending maintainer verification against test infrastructure. Refs bytefolk#173
PeterGuy326
left a comment
There was a problem hiding this comment.
Gate decision
REQUEST CHANGES on current head b5150a3a0e5fc496036fa46516c98c732b7f43f6.
Blocking findings
- This branch is stacked on the live recall producer work from
#184:git log origin/main..origin/pr-180includes thebenchmarks/recallproducer, tests, and README. That scope is unrelated to HNSW and must be split or explicitly rebased so#180does not merge#184a second time. - The HNSW migration is numbered
0024, while open#183also adds0024_files_lexical_search.sql. Coordinate migration numbering/rebase before either PR merges. scripts/verify_hnsw_indexes.shuses vector literals containing..., which are not valid pgvector literals, and the script only prints EXPLAIN output without asserting an index scan. The verification recipe therefore cannot currently prove its stated acceptance criteria.
Please push a scoped branch, resolve the migration collision, and make the verification script executable against real vectors with assertions (or mark those checks explicitly as not verified).
- Remove benchmark/recall files (belong to bytefolk#184, not this PR) - Rename migration 0024 → 0025 to resolve collision with bytefolk#183 - Fix verify script: use valid pgvector literals via array_fill, add index-scan assertion instead of just printing EXPLAIN output - Update VALIDATION_HNSW.md references to 0025
- Remove bytefolk#184 stacking (recall benchmark is a separate PR) - Rename migration 0025 → 0026 to avoid collision with bytefolk#180
|
Current-head CI approval readback (2026-09-10) Head:
Failures at this snapshot: |
|
Superseded by @PeterGuy326's #197 which covers the same changes on current main. Closing per the '本轮先不合并' directive. |
Summary
embeddings_text(768-d),embeddings_visual(512-d), andembeddings_face(512-d)vector_cosine_ops0001_init.sqland0019_versioned_index_generations.sqlto reference the new migrationface.gopackage comment to reflect that HNSW index is now availableTest plan
scripts/verify_hnsw_indexes.shdocs/VALIDATION_HNSW.mdscripts/verify_hnsw_indexes.shagainst test database with populated corpusValidation Documentation
See
docs/VALIDATION_HNSW.mdfor:Files Changed
server/internal/db/migrations/0024_ann_hnsw_indexes.sql— HNSW indexesserver/internal/db/migrations/0001_init.sql— updated deferral commentserver/internal/db/migrations/0019_versioned_index_generations.sql— updated deferral commentserver/internal/face/face.go— updated package commentscripts/verify_hnsw_indexes.sh— automated EXPLAIN verificationdocs/VALIDATION_HNSW.md— validation ledgerFixes #173