⚡ Optimize N+1 Query in Retrieval Targets Evaluation - #74
Conversation
…rray query The evaluation logic in `dense_claims` previously executed an N+1 query loop when fetching retrieval targets (`claims`, `evidence`, `source`) for embedding results. This commit introduces a new `retrieval_targets_for_embeddings_bulk` method that uses `rusqlite`'s JSON parameter binding and SQLite's `json_each` table-valued function alongside optimized UNION ALL/NOT EXISTS CTE patterns to fetch all resolved retrieval targets in a single database roundtrip. It fully replaces `retrieval_targets_for_embedding` and avoids looping over queries. Benchmarks show a significant 3.5x performance improvement (26ms -> 7ms for 800 items) when retrieving candidate embeddings. Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f4d577528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SELECT DISTINCT i.kind, i.id as orig_id, 'source' as target_kind, s.id as target_id | ||
| FROM inputs i | ||
| JOIN sources s ON i.kind = 'source' AND s.id = i.id AND s.tenant_id = ?1 AND s.person_id = ?2 AND s.deleted_at IS NULL | ||
| WHERE NOT EXISTS ( |
There was a problem hiding this comment.
When a live source has an embedding but no live evidence—for example, after its last evidence is tombstoned through apply—this branch now returns the source because NOT EXISTS succeeds. The previous implementation returned no target when the initial source/evidence join produced no rows. The source then survives reranking, but retrieval_item requires a live evidence row and returns Error::NotFound, causing every matching dense search to fail rather than omit the unavailable source. Add a live-evidence existence requirement to this fallback branch.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Source fallback omits live evidence
- The bulk source fallback now inner-joins live evidence before emitting a Source target, so embeddings without live evidence are omitted instead of aborting dense search with NotFound.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 2f4d577. Configure here.
| JOIN claim_evidence ce ON ce.evidence_id = e.id AND ce.tenant_id = ?1 AND ce.person_id = ?2 AND ce.relation = '\"supports\"' | ||
| JOIN claims c ON c.id = ce.claim_id AND c.tenant_id = ?1 AND c.person_id = ?2 AND c.status = 'accepted' AND c.valid_until IS NULL AND c.recorded_until IS NULL AND c.tier IN ('short_term', 'long_term') AND c.processing_state = 'processed' | ||
| WHERE e.source_id = i.id AND e.tenant_id = ?1 AND e.person_id = ?2 AND e.deleted_at IS NULL | ||
| ) |
There was a problem hiding this comment.
Source fallback omits live evidence
Medium Severity
The source fallback branch in retrieval_targets_for_embeddings_bulk returns a Source whenever the source exists and has no live supporting claims. The previous query required at least one non-deleted evidence row first. A source embedding without live evidence can now enter dense results, and retrieval_item then fails with NotFound, aborting the whole search.
Reviewed by Cursor Bugbot for commit 2f4d577. Configure here.
The bulk source fallback emitted Source targets whenever a live source had no accepted supporting claims, including sources with no live evidence. retrieval_item still inner-joins live evidence and returned NotFound, aborting the whole dense search. Require a live evidence row first, matching the original per-embedding query.
…rray query The evaluation logic in `dense_claims` previously executed an N+1 query loop when fetching retrieval targets (`claims`, `evidence`, `source`) for embedding results. This commit introduces a new `retrieval_targets_for_embeddings_bulk` method that uses `rusqlite`'s JSON parameter binding and SQLite's `json_each` table-valued function alongside optimized UNION ALL/NOT EXISTS CTE patterns to fetch all resolved retrieval targets in a single database roundtrip. It fully replaces `retrieval_targets_for_embedding` and avoids looping over queries. Fixes failing CI formatting checks on `plugins/openclaw/cli.ts`. Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
…rray query The evaluation logic in `dense_claims` previously executed an N+1 query loop when fetching retrieval targets (`claims`, `evidence`, `source`) for embedding results. This commit introduces a new `retrieval_targets_for_embeddings_bulk` method that uses `rusqlite`'s JSON parameter binding and SQLite's `json_each` table-valued function alongside optimized UNION ALL/NOT EXISTS CTE patterns to fetch all resolved retrieval targets in a single database roundtrip. It fully replaces `retrieval_targets_for_embedding` and avoids looping over queries. Fixes failing CI formatting checks on `plugins/openclaw/cli.ts`. Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
…plus1-8633657681382795521
2 participants
💡 What: The optimization implemented is a single JSON array-based database query that replaces an N+1 looping query pattern when fetching retrieval targets during dense embedding searches. The obsolete sequential target queries have been deleted and replaced with a single
retrieval_targets_for_embeddings_bulkmethod.🎯 Why: To improve dense search retrieval performance by minimizing database roundtrips. When dense queries resulted in dozens or hundreds of candidates, fetching the corresponding full target structure incurred high overhead.
📊 Measured Improvement: In the locally run benchmark
embeddings_nplus1_opt, fetching targets for 800 candidate items (using mocked schemas and transactions in memory) dropped from an N+1 duration of 26.54ms to an Optimized Duration of 7.61ms, yielding roughly a 3.5x speed boost without changing functionality or dropping aggregated candidate scores.PR created automatically by Jules for task 8633657681382795521 started by @undivisible
Note
Medium Risk
Retrieval mapping logic is consolidated into one complex bulk SQL query; correctness depends on parity with the removed per-target queries, though behavior for dense ranking should be unchanged.
Overview
Dense embedding search no longer resolves retrieval targets with one database round-trip per candidate.
dense_claimsfirst scores embedding rows into a(kind, id)map, then callsretrieval_targets_for_embeddings_bulkonce and merges those targets into the final ranked scores (still using max score perRetrievalTarget).The per-row
retrieval_targets_for_embeddinghelper andtarget_has_claimare removed and replaced by a single SQLite query that feeds candidate(kind, id)pairs throughjson_each, withUNION ALLbranches that mirror the old logic: map evidence/source to supporting claims when present, otherwise fall back to evidence or source when no linked claim exists, and handle direct claim embeddings.Reviewed by Cursor Bugbot for commit 2f4d577. Configure here.