fix: Index.fts({ withPosition: true }) to enable phrase queries#404
Conversation
CI 分析:此 PR 的 FTS 修正與 CI 失敗無關CI 失敗的真正原因本次 CI 失敗的是 這是 upstream 觸發原因測試 config 為 Upstream master 最近有兩個 commit 造成 regression:
當 selfImprovement.enabled 預設為 true 時, upstream master 的 CI 狀態master 分支本身的 CI 也處於 failed 狀態(最近兩次 push 都是 failed),確認這是既有問題,不是本 PR 引入的。 本 PR 的變更確認
建議等待 upstream maintainer 修復 master 的 selfImprovement regression 後,本 PR 即可合併。 |
AliceLJY
left a comment
There was a problem hiding this comment.
LGTM — changes are clean, on-topic, and well-tested. Approving.
Issue #402 — Full Bug Report (for PR reference)Environment
ErrorEvery BM25 query fails with this error, causing a fallback to empty results: Root CauseIn await table.createIndex("text", {
config: (lancedb as any).Index.fts(), // withPosition defaults to false
});
The static fts(
withPosition?: boolean, // ← first arg, defaults to false
baseTokenizer?: string,
language?: string,
...
): IndexImpact
WorkaroundThe plugin already has Suggested FixOption A — Fix at creation time (preferred, prevents future occurrences): // src/store.ts, createFtsIndex()
config: (lancedb as any).Index.fts(true), // explicitly enable position dataOption B — Add rebuildFtsIndex to auto-trigger when FTS query fails with this specific error, instead of just falling back silently. API Signature Correction (CRITICAL)
Why
|
AliceLJY
left a comment
There was a problem hiding this comment.
Clean fix, well-researched — especially the catch on Index.fts(true) vs Index.fts({ withPosition: true }). The options-object API is a subtle footgun and your analysis in the comments is excellent.
One minor note for release: existing users who already have an FTS index without position data will need to call rebuildFtsIndex() once after upgrading, since this fix only applies to newly created indexes. Worth a one-liner in the changelog.
CI failure is clearly unrelated (upstream selfImprovement regression on master, tracked in #405). LGTM!
Review: Index.fts({ withPosition: true }) to enable phrase queriesVerdict: approve (conditional) | Confidence: 0.90 | Value: 65% One-line fix, correct and high-impact — phrase queries were silently broken for all BM25 users. Code LGTM. Before merge
No code changes neededThe fix itself is correct and complete for new indexes. The only gap is telling existing users how to remediate. Auto-reviewed by auto-pr-review-orchestrator | 6 rounds | Claude + Codex adversarial |
507994e to
2186131
Compare
Re-apply fix after rebase onto latest upstream/master (3dc0956).
|
Both requests addressed:
Existing users upgrading will still get broken phrase queries if their FTS indexes were created without position data. They need to run: openclaw memory-pro reindex-ftsOr call Ready for merge. Thanks for the review @rwmjhb! |
Problem
BM25 phrase queries (e.g.
"exact phrase") were silently broken because FTS indexes were created without position data (withPosition: falseis the default).Fix
Change
Index.fts()toIndex.fts({ withPosition: true })insrc/store.ts.This adds position data to the FTS index, enabling phrase queries to work correctly.
Changelog
Index.fts({ withPosition: true })enables phrase queriesUpgrade Note
Existing users with old FTS indexes will still get broken phrase queries after upgrade. To fix, run:
Or call
rebuildFtsIndex()programmatically. This rebuilds the index with position data so phrase queries work correctly.Verification
Index.fts()creates index without position data → phrase queries silently failIndex.fts({ withPosition: true })creates index with position data → phrase queries workCloses #402