Skip to content

perf(index): add HNSW ANN indexes for all embedding tables - #180

Closed
sun-970 wants to merge 3 commits into
bytefolk:mainfrom
sun-970:fix/173-hnsw-ann-index
Closed

sun-970 wants to merge 3 commits into
bytefolk:mainfrom
sun-970:fix/173-hnsw-ann-index

Conversation

@sun-970

@sun-970 sun-970 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add pgvector HNSW indexes with cosine distance on embeddings_text (768-d), embeddings_visual (512-d), and embeddings_face (512-d)
  • Vector queries now use index scans instead of sequential scans of the entire embedding table
  • Migration 0024 creates the three indexes using vector_cosine_ops
  • Update deferral comments in 0001_init.sql and 0019_versioned_index_generations.sql to reference the new migration
  • Update face.go package comment to reflect that HNSW index is now available
  • Add validation script and ledger documenting EXPLAIN regression test methodology

Test plan

  • Verification script created: scripts/verify_hnsw_indexes.sh
  • Validation ledger documented: docs/VALIDATION_HNSW.md
  • Maintainer action required: Run scripts/verify_hnsw_indexes.sh against test database with populated corpus
  • Confirm EXPLAIN shows index scans (not seq scans) for text and visual queries
  • Verify query latency improvement on realistic corpus (10k+ vectors)
  • Confirm recall is unchanged (expected >95% with default HNSW parameters)

Validation Documentation

See docs/VALIDATION_HNSW.md for:

  • Test methodology and expected EXPLAIN output
  • Before/after query plan comparison
  • Failure modes (inconsistent dimensions, empty tables, NULL embeddings)
  • Recall measurement approach

Files Changed

  • server/internal/db/migrations/0024_ann_hnsw_indexes.sql — HNSW indexes
  • server/internal/db/migrations/0001_init.sql — updated deferral comment
  • server/internal/db/migrations/0019_versioned_index_generations.sql — updated deferral comment
  • server/internal/face/face.go — updated package comment
  • scripts/verify_hnsw_indexes.sh — automated EXPLAIN verification
  • docs/VALIDATION_HNSW.md — validation ledger

Fixes #173

)

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 PeterGuy326 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate decision

REQUEST CHANGES on current head b5150a3a0e5fc496036fa46516c98c732b7f43f6.

Blocking findings

  1. This branch is stacked on the live recall producer work from #184: git log origin/main..origin/pr-180 includes the benchmarks/recall producer, tests, and README. That scope is unrelated to HNSW and must be split or explicitly rebased so #180 does not merge #184 a second time.
  2. The HNSW migration is numbered 0024, while open #183 also adds 0024_files_lexical_search.sql. Coordinate migration numbering/rebase before either PR merges.
  3. scripts/verify_hnsw_indexes.sh uses 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
sun-970 pushed a commit to sun-970/mem that referenced this pull request Sep 10, 2026
- Remove bytefolk#184 stacking (recall benchmark is a separate PR)
- Rename migration 0025 → 0026 to avoid collision with bytefolk#180
@PeterGuy326

Copy link
Copy Markdown
Collaborator

Current-head CI approval readback (2026-09-10)

Head: 6cd02451dfd586119763b9b8afb6918081d42322. The original pending-approval runs on this head have been approved and real check-runs now exist (17 at this snapshot). No old-head run was approved. Auto-merge remains disabled; this is not a code approval or merge-ready declaration.

Failures at this snapshot:

@sun-970

sun-970 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by @PeterGuy326's #197 which covers the same changes on current main. Closing per the '本轮先不合并' directive.

@sun-970 sun-970 closed this Sep 10, 2026
waterbro-8 pushed a commit that referenced this pull request Sep 18, 2026
- Remove #184 stacking (recall benchmark is a separate PR)
- Rename migration 0025 → 0026 to avoid collision with #180
Sign up for free to 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.

perf(index): add an ANN index so vector search stops scanning every embedding

3 participants