Skip to content

perf(core): bound temporal search context lookup - #139

Open
kongtou20070406 wants to merge 2 commits into
tommy0103:mainfrom
kongtou20070406:perf/indexed-search-context
Open

perf(core): bound temporal search context lookup#139
kongtou20070406 wants to merge 2 commits into
tommy0103:mainfrom
kongtou20070406:perf/indexed-search-context

Conversation

@kongtou20070406

@kongtou20070406 kongtou20070406 commented Sep 3, 2026

Copy link
Copy Markdown

What and why

search() currently runs one temporal-context query per FTS hit. That query
orders every other message in the hit session by a JULIANDAY() expression, so
SQLite cannot use idx_messages_ts; a hit in a 102,648-message session costs
about 206-235 ms just to collect six neighbors.

This change bounds that work without changing the result contract:

  • canonical provider timestamps use idx_messages_ts to take at most six rows
    from each side of the hit (plus at most six null-timestamp rows);
  • the final 18-or-fewer candidates retain the prior JULIANDAY() distance and
    rowid tie ordering;
  • null or non-canonical hit timestamps keep the original scan;
  • both statements are prepared once per search() call instead of once per hit.

No schema, dependency, provider, visibility, or public API changes.

Reproduction and ablation

Windows 11, Node 24.16.0, immutable 3.33 GB SQLite snapshot with 1,627,293
messages. Each value is the median of five warm search(query, { limit: 20 })
runs. "Disabled" is current parent fb4a8ef; "enabled" is this commit.

query disabled p50 enabled p50 reduction
索引 863.70 ms 9.66 ms 98.9%
index 3,657.46 ms 23.31 ms 99.4%
error 3,783.40 ms 30.60 ms 99.2%
python 1,174.78 ms 51.94 ms 95.6%
obelisk 3,344.95 ms 38.67 ms 98.8%
EqOp 998.55 ms 76.22 ms 92.4%

The six ordered hit-ID digests were identical with the optimization disabled
and enabled. An old-vs-new temporal-context differential over 120 real hits had
0 context UUID/order mismatches. The new regression test separately pins null
timestamps, duplicate timestamps, meta filtering, and inactive/hidden rows.

Verification

  • npm test — 659 pass / 0 fail on Ubuntu WSL2 (Node 24.14.1)
  • npm run typecheck — 0 errors (root + app)
  • npm run lint — 0 errors, 11 pre-existing/generated warnings
  • New test runs in root CI (tests/query.test.mjs is matched by tests/*.test.mjs)
  • No existing assertion was loosened
  • Re-ran the checks above on the current head

The current-head query suite also passes 32/32 on Windows. Linux verification
was rerun after rebasing onto fb4a8ef.

Deliberately out of scope

  • Provider discovery/index refresh and FTS rebuild latency; this PR only fixes
    post-FTS result context lookup.
  • Adding another index: the existing idx_messages_ts(session_id, timestamp) is
    sufficient.
  • Electron tests, because no app/ files changed.

Copilot AI lite review requested due to automatic review settings September 3, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The context sorting comparator in query.ts violates the JS sort contract for equal/null timestamps, which can yield unstable or incorrect ordering in the new temporal-context scenarios.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR optimizes createQueryApi().search() temporal-context lookup by avoiding per-hit full-session scans and instead bounding candidate selection via the existing (session_id, timestamp) index while preserving the prior distance/tie ordering behavior for canonical timestamps.

Changes:

  • Prepare and reuse two temporal-context statements per search() call (indexed path for canonical timestamps; scan fallback otherwise).
  • Bound temporal neighbor candidates to a small window (plus limited null-timestamp rows) before applying JULIANDAY() distance ordering.
  • Add a regression test covering null timestamps, duplicate timestamps, and visibility/meta filtering behavior.
File summaries
File Description
packages/core/src/query.ts Adds bounded, index-assisted temporal context retrieval with a scan fallback and prepared-statement reuse.
tests/query.test.mjs Extends search tests to validate neighbor selection around null and duplicate timestamps; aligns test DB with the production timestamp index.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/core/src/query.ts Outdated
const ctx = (indexedTimestamp ? indexedContext : scanContext)
.all(contextParams)
.map(withVisibility)
.sort((a: DbRow, b: DbRow) => a.timestamp < b.timestamp ? -1 : 1);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in 5d89ce8. Equal timestamps now return 0, null timestamps have an explicit null-first order, and non-null ISO timestamps remain ascending. The regression test now asserts the exact duplicate/null order instead of sorting UUIDs before comparison. Verification: query suite 32/32; old-SQL vs indexed-context differential 120/120 real hits with 0 mismatches.

@kongtou20070406
kongtou20070406 force-pushed the perf/indexed-search-context branch from 6f58273 to f0eaa08 Compare September 3, 2026 10:02
@kongtou20070406

Copy link
Copy Markdown
Author

Addressed Copilot's comparator-contract finding in 5d89ce8. Equal timestamps now return 0, null ordering is explicit, and the regression pins exact null/duplicate-timestamp order. Current-head verification: Linux 659/659 tests, typecheck clean, lint 0 errors (11 pre-existing/generated warnings), Windows query suite 32/32, and 120/120 real-hit old-vs-new context comparisons with 0 UUID/order mismatches. As a fork author I do not have upstream RequestReviewsByLogin permission; could a maintainer please re-request Copilot review?

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.

2 participants