Skip to content

fix: preserve vector-first ANN query plan - #257

Merged
NaNomicon merged 2 commits into
tickernelz:mainfrom
lindixu6-hash:fix/vector-search-query-plan
Aug 22, 2026
Merged

fix: preserve vector-first ANN query plan#257
NaNomicon merged 2 commits into
tickernelz:mainfrom
lindixu6-hash:fix/vector-search-query-plan

Conversation

@lindixu6-hash

Copy link
Copy Markdown
Collaborator

Summary

  • use CROSS JOIN in both vector_top_k query paths so SQLite keeps the virtual table before memories
  • preserve the existing filtered and unfiltered result semantics
  • add a regression test covering both SQL branches

Why

Issue #247 shows SQLite can reorder the current inner join, drive from memories, and evaluate vector_top_k once per matching row. CROSS JOIN acts as the planner barrier proposed in the issue while keeping the join predicate and filters unchanged.

Closes #247.

Verification

  • bun run typecheck
  • bun run format:check
  • bun run build
  • bun test (408 pass, 0 fail)

@lindixu6-hash

Copy link
Copy Markdown
Collaborator Author

Remote validation for the exact PR commit c12b4e3334ca80e58acd677ad28f1f61fc3cb10d: Platform Package Smoke run 32068871957 passed all six jobs (Ubuntu, Windows, macOS Apple Silicon 15/26, and macOS Intel 15/26), including install, typecheck, build, the full test suite, and package-install smoke.

For transparency, the first Windows attempt hit the existing 5-second cleanup timeout in turso-reembed-migration.test.ts; the vector-search tests passed in that attempt, and the Windows rerun completed fully green without code changes.

@lindixu6-hash

Copy link
Copy Markdown
Collaborator Author

Hi @EyJunge1, when convenient, could you independently review this PR and the related diagnostic fix in #258? You are the recent merger for the Turso, auto-capture, dependency, and v2.24.x release work, so these two paths align with your recent context.

Both PRs are limited to two files, remain mergeable, and have exact-SHA six-platform package-smoke evidence. I will not self-approve or merge either change.

@EyJunge1

Copy link
Copy Markdown
Contributor

Thanks for the ping! Honestly, I'm no longer maintaining this project and don't have the time to give this PR a proper review. Better to loop in one of the active maintainers who can do it justice. Appreciate your understanding.

@lindixu6-hash

Copy link
Copy Markdown
Collaborator Author

Fresh validation after updating this branch to cumulative latest main: exact PR head 70eb41ce7998812e06cc626ee6376443bc2ea1a4 passed Platform Package Smoke run 32281742023 on Ubuntu, Windows, macOS 15/26 Apple Silicon, and macOS 15/26 Intel, plus GitGuardian. The PR remains independently review-gated; I will not self-approve or merge it.

@NaNomicon

Copy link
Copy Markdown
Collaborator

#258 (comment)

@NaNomicon

Copy link
Copy Markdown
Collaborator

Review — PR #257: "fix: preserve vector-first ANN query plan"

Verdict: SHIP. Verified the planner-barrier fix against real libSQL. Minimal, correct, no blast radius.

The fix: JOINCROSS JOIN in both searchKind SQL branches (src/services/turso/vector-search.ts:239,245). CROSS JOIN is SQLite's documented optimizer barrier — it forces vector_top_k (virtual table) to stay the driving table so it is evaluated once, not once per matching memories row. Join predicate m.rowid = v.id and all WHERE filters unchanged.

What is actually fixed (the interesting part)

Only the filtered branch was broken. Confirmed with EXPLAIN QUERY PLAN against real libSQL (v0.5.29):

OLD filtered:  SEARCH m USING INDEX idx_container_tag   <- memories drives, vector_top_k per row (bug #247)
NEW filtered:  SCAN v VIRTUAL TABLE ...                 <- vector_top_k drives, fixed

The unfiltered branch already picked v-first naturally — CROSS JOIN is a no-op there but harmless.

Why it is safe

CROSS JOIN with an ON clause is semantically identical to INNER JOIN — it only constrains planner join order, not the result set. Verified:

  • Real-execution test (inserts and searches memories with native vector index) drives the modified query against a real SQLite shard — passes, correct ranking.
  • Row-count equivalence at production k (128 filtered / 32 unfiltered): old and new return identical result sets.
  • Graceful fallback intact: CROSS JOIN against a missing index still throws into exactScanKind fallback.
  • No twin-copy miss: only 2 JOIN memories in the codebase, both changed.
  • m.rowid = v.id is correct — memories has a TEXT PRIMARY KEY, so rowid is the physical rowid (not aliased to the PK), and vector_top_k returns those integer rowids. Integer-to-integer join, unaffected by the keyword change.

On the test

The added test (tests/turso-vector-search.test.ts) mocks db.all and asserts the emitted SQL shape — it guards against revert but does not execute SQLite. That is fine here because the existing real-SQLite test already exercises the modified query. 2/2 pass.

Optional hardening (not required): add an EXPLAIN QUERY PLAN assertion to the real-SQLite test checking the filtered plan drives from vector_top_k — would catch a future libSQL/SQLite version that stops honoring the CROSS-JOIN barrier. Belt-and-suspenders only.

Merge gate

  • Correct, minimal, fail-safe (graceful fallback preserved)
  • Real-SQLite execution verified
  • Test guards the revert
  • No breakage beyond the diff (verified callers, triggers, fallback)

Merge as-is. Closes #247 properly. (Verified via static code + EXPLAIN QUERY PLAN + real execution; live behavior across SQLite versions relies on the CROSS-JOIN planner-barrier semantics, which are standard SQLite.)

@lindixu6-hash

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough independent review and the real-libSQL plan verification. I agree the EXPLAIN QUERY PLAN assertion is useful optional hardening, but keeping it out preserves this PR's narrow scope.

All checks remain green. Since this is my PR, I will not self-merge it; please merge when convenient.

@NaNomicon
NaNomicon merged commit f5b15f5 into tickernelz:main Aug 22, 2026
7 checks passed
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.

[Bug]: memory search takes ~8s when filtering by container_tag (vector_top_k + JOIN bad query plan)

3 participants