feat(ts-oss): add Qdrant server-side BM25 keywordSearch() + filter indexes - #5851
Conversation
Changes RequestedGreat work on this — the capability-gated design is exactly right, the test coverage is thorough (30 tests, all passing), and the backward-compatibility path for pre-existing collections is solid. Two small asks before merge. Strengths
IssuesImportant (should fix before merge)
Minor (optional)
Evidence
AssessmentReady to merge: With fixes — please address the two Important items (a comment + one test); they're both small. Happy to re-review once pushed. |
|
Thanks, will fix accordingly, and acknowledge, thanks for the timely reviews . |
…h path, and suggested comments
|
Addressed both required items:
Also picked up the three optional nits: the constructor _isRemote comment, console.debug in createFilterIndexes, and a note on the Looking forward to further reviews and fixes (if any). Thanks |
|
Re-review @ 4d35115 — Changes Requested (Prettier only) Both Important items and all three optional nits: ✓ addressed. Verified fixed:
One remaining blocker — CI Prettier failure:
Run Evidence:
|
|
Sure, working on it |
|
Pushed the prettier fix - CI Gate is waiting on workflow approval whenever you get a chance. Thanks !! |
|
Verified against 2ddd158 — Prettier is clean. `mem0-ts/src/oss/src/vector_stores/qdrant.ts:5–9` — double blank line and trailing space both gone: `npx prettier --check src/oss/src/vector_stores/qdrant.ts` → All matched files use Prettier code style! LGTM — ready for human review & squash-mergeWhat this PR doesReplaces the null stub `keywordSearch()` on the TypeScript Qdrant store with a real BM25 hybrid-search implementation backed by Qdrant's server-side inference (≥ 1.15.2), including capability-gated collection setup, payload index creation, and a dense-path regression test. Strengths
IssuesMinor: none remaining (nits addressed in prior push). Evidence
AssessmentReady to merge: Yes (squash) · Est. human time: ~2 min |
|
Hi @kartik-mem0, hope you're doing well! Just wanted to check if there are any updates on the PR. If there are any remaining changes or anything else I can do to help move it forward, please let me know. Thanks! |
|
Hi @kartik-mem0 |
…ord-search # Conflicts: # mem0-ts/src/oss/src/vector_stores/qdrant.ts
|
Hi @kartik-mem0, the conflicts have been resolved with respect to the recent client refactoring on main. I've integrated the BM25 setup and tests into the new ensureClient() flow. Please do verify and help us get it merged. |
Changes Requested @ d1d9dae (reversing my earlier LGTM)@bmsvinci1729 first: apologies for the delay, and thank you for staying on top of every round including the main merge. My previous review said "ready to merge" with the caveat "Could NOT verify: live Qdrant >= 1.15.2 server behavior, code tracing only." I've now run this against real Qdrant servers in Docker, and that verdict was wrong. There is a silent data-loss path that code reading alone did not surface. Everything below is from actual runs, not inspection. What works (verified on self-hosted Qdrant v1.18.2)The good news up front: server-side
The capability-gate design is right, and the backward-compat path for pre-existing collections genuinely holds up under a real server. No complaints there. Blocker: silent data loss when Qdrant inference is unavailableSame code, same script, against Qdrant v1.14.1: Root cause: await this.client.createCollection(name, createParams as any);
if (enableBm25) {
this._hasBm25Slot = true; // <- optimistic; server may not support inference
}Older servers accept the Why this is silent rather than loud: try {
await this.vectorStore.insert(allVectors, allIds, allPayloads);
} catch {
for (let i = 0; i < allIds.length; i++) {
try { await this.vectorStore.insert([allVectors[i]], [allIds[i]], [allPayloads[i]]); }
catch (e) { console.error(`Failed to insert memory ${allIds[i]}: ${e}`); } // swallowed
}
}I ran that exact loop against 1.14.1: So Who is affected (new collections only; pre-existing collections without the slot stay on the safe path):
Point 2 is why a server-version check will not close this: the version can be 1.18 and inference still be unavailable. Suggested fixMake the capability detection reactive instead of optimistic: if an upsert fails while private async upsertPoints(points: any[]): Promise<void> {
try {
await this.client.upsert(this.collectionName, { points });
} catch (error) {
if (!this._hasBm25Slot) throw error;
// Server accepted the bm25 slot but cannot run inference (Qdrant < 1.15.2,
// or a Cloud cluster with inference disabled). Downgrade permanently and retry
// dense-only so writes are never lost.
this._hasBm25Slot = false;
console.warn(
"Qdrant rejected the server-side BM25 vector; falling back to dense-only inserts. " +
"BM25 keyword scoring requires Qdrant >= 1.15.2 with inference enabled.",
error,
);
await this.client.upsert(this.collectionName, {
points: points.map((p) => ({ ...p, vector: p.vector?.[""] ?? p.vector })),
});
}
}Then route both Worth noting Python already degrades correctly here for free: it encodes client-side with fastembed, so a missing encoder just omits the sparse vector and the insert still lands. Staying with server-side inference in TS is still the right call (no heavy dependency, and no JS BM25 encoder matches Qdrant's tokenization); it just needs the same "never lose the write" property. Minor, non-blocking
Evidence
AssessmentScore: 7/10. Ready to merge: not yet, one fix. The feature is real, correct on supported servers, and the test coverage is better than most PRs this size. The single blocker is the optimistic |
`ensureCollection` set `_hasBm25Slot = true` as soon as `createCollection` resolved. Servers that cannot run server-side inference still accept the `sparse_vectors` config (sparse vectors predate inference), so the guard never tripped and every subsequent upsert failed with HTTP 500 "InferenceService is not initialized". `memory/index.ts` swallows per-point insert errors, so `add()` returned success with memory IDs that were never persisted. Route `insert()` and `update()` through `upsertPoints()`, which downgrades `_hasBm25Slot` and retries once dense-only when the server rejects the BM25 vector. A version check cannot cover this: Qdrant Cloud enables inference by default only for clusters created after 2025-07-07, so a 1.18 cluster can still have it off. Verified against qdrant/qdrant:v1.14.1 (inference absent): inserts now persist and `keywordSearch()` returns null, and v1.18.2 is unchanged (BM25 scores intact, no warning).
Pushed the fix myself + retracting one of my findings@bmsvinci1729 you've been through enough rounds on this one, so rather than send you back for another, I merged First, a correction: my
|
v1.18.2 (inference on) |
v1.14.1 (no inference) |
|
|---|---|---|
insert() |
no throw, 0 warnings | no throw, 1 warning |
| points persisted | 3 | 3 (was 0 on your branch) |
| stored vector shape | ["bm25", ""] |
plain dense array |
keywordSearch("espresso") |
0.7868 / 0.7847 |
null |
filtered keywordSearch |
narrowed to alice |
null |
search() dense |
[1, 0.6302, 0.2387] |
identical |
| 2nd insert | 0 extra warnings | 0 extra warnings |
update() re-encode |
old term drops, new term searchable | null, write lands |
| payload indexes | 4 keyword indexes | 4 keyword indexes |
memory_migrations |
{} |
{} |
The silent-loss path specifically. Replaying memory/index.ts's two-level insert fallback against v1.14.1:
before: => add() would now RETURN SUCCESS with ids: [...]
=> actual points stored: {"count":0}
after: => add() reports success for: 2 ids
=> points ACTUALLY persisted: {"count":2}
Tests. 31 -> 36 in qdrant-keyword-search.test.ts. The 5 new ones cover: insert downgrades and retries with a plain array; update does the same; a legacy-collection network error still rejects with only one upsert attempt; the next insert succeeds first try; memory_migrations gets zero createPayloadIndex calls (the regression lock for my bad finding above). Stashing the fix turns exactly 3 of them red.
npx jest src/oss-> 1224 passed, 78 suitesnpx prettier --check-> cleanpnpm run build-> ok
Repo sandbox, end-to-end through Memory.add() + search() on the oss-qdrant lane:
main: FAIL (3.4s): bm25Score is 0 for query "invoice" ...
this branch: PASS (3.8s)
Assessment
9/10, merging. The feature was right, the design was right, and the test coverage was already above average for a PR this size. The one real defect was a fallback that couldn't have shown up without a server that accepts the config but can't act on it. Thanks for the persistence through the review rounds, and again, sorry for the wrong minor finding and the delay.
|
Thanks for pushing that fallback fix, and for this whole journey -- my first open source contribution, learned a lot along the way! Appreciate all the review effort here !! |
Summary
Implements
keywordSearch()for the TypeScript OSS Qdrant adapter, which was previously anullstub. So hybrid search on Qdrant silently ran as semantic + entity only, with no BM25 keyword signal.The following PR brings TS Qdrant to parity with the other TS stores (PGVector, Azure AI Search) and with the Python Qdrant adapter.
Closes #5828
Direction was confirmed by a maintainer in #5828 (server-side BM25, Qdrant ≥ 1.15.2 floor, payload-index fix in the same PR).
What changed (
mem0-ts/src/oss/src/vector_stores/qdrant.ts)bm25named sparse-vector slot withmodifier: "idf"so Qdrant computes IDF over the live corpus.{ text, model: "Qdrant/bm25" }).bm25slot (using: "bm25") and return{ id, payload, score }[]bm25slot. Collections created before this change lack it ->keywordSearch()returnsnull(existing behavior) and aconsole.warnis logged, also no BM25 vectors are written, so semantic search is unchanged, thus ensuring no breakage on upgrade.keywordpayload indexes for["user_id", "agent_id", "run_id", "actor_id"]at collection setup (skipped for local/embedded mode, best-effort per field). Without these, filtered keyword/semantic search is rejected by remote Qdrant ("Index required but not found"). Mirrors Python's_create_filter_indexes().Requirements
Qdrant/bm25model.Tests
Added
qdrant-keyword-search.test.ts(30 tests, following the existingqdrant-url-port.test.tsstyle) covering collection setup (bm25 slot, 409/401/403 paths, dimension guard), insert/update BM25 attachment, query result mapping, gracefulnullfallback on errors, and filter construction.Also validated end-to-end against a real Qdrant Cloud cluster (v1.18.2): insert -> server-side BM25 encode -> keyword query -> correctly ranked results, including the metadata-filtered case. Happy to share that script if useful.
Type of Change
Checklist