Repository navigation
perf: make a date-filtered search bound its own work - #84
Conversation
Migration 010. Both FTS indexes gain a search_period column, fed by a VIRTUAL generated column on turns and tool_uses that slices the day and month out of the stored timestamp. Nothing queries it yet; the next commit compiles date filters into terms that match it. Derived rather than stored, so there is no backfill UPDATE and the token cannot drift from the timestamp it describes. Sliced out of the text rather than read with strftime because the date filter is a lexicographic comparison of that same text — deriving the token from it is what ties the token to the comparison, so a row the predicate admits always carries a token in the compiled set. A timestamp that is not a padded date gets the 'ccvymx' sentinel, which every compiled term set includes. Without it such a row would vanish from date-filtered searches instead of being decided by the predicate, which would trade a slow answer for a wrong one. An FTS5 column list cannot be altered, so both indexes are dropped and rebuilt. turns_fts as well as tool_uses_fts: the UNION has two branches and leaving one unbounded leaves the cost growing with the archive. That rebuild also discards the orphaned turns_fts entries issue #81 reports — 29,268 of them on the author's archive, from the era before the turns_ad trigger's recursive_triggers dependency was understood. turns_au and tool_uses_au now fire on timestamp as well. The period token derives from it, so an UPDATE that moved a row in time and left its text alone would otherwise leave the index claiming the wrong month. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… for Both branches of the text_hits UNION now carry the period terms migration 010 indexed, so a date filter prunes inside the postings-list intersection instead of after the whole all-time match set has been materialised into a temp B-tree. The timestamp predicate stays exactly where it was: the terms make the scan bounded, the predicate keeps the answer right, and it is the predicate that trims the partial first and last day to the hour. A one-sided filter takes its other side from the archive's own date range, read with two single-aggregate queries that SQLite answers with a seek on idx_turns_timestamp. after: with no before: is the common case and has no upper bound of its own; using "now" for it would silently drop any row stamped in the future, and a clock-skewed transcript happens. Every MATCH this package builds is now column-scoped — the caller's text against the content columns, period terms against search_period. That is what keeps a turn whose text happens to hold a period token out of a date filter, and a caller searching for that token from matching the whole month. db.SearchTurns and SearchTurnsWithFilters are scoped the same way for the same reason. The match-attribution subqueries keep the unpruned expression. They are already scoped to a single turn, so they have nothing to prune, and leaving the period terms out of them keeps snippet()'s automatic column choice looking only at the two columns that hold payload text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ndex Pruning the index turned out to buy 12% rather than the order of magnitude it should have, because the index scan was not where the time was going. Measured on the author's archive, `git after:2026-10-01`: text_hits CTE, unpruned 0.110s text_hits CTE, pruned 0.001s whole statement, before 1.19s whole statement, pruned CTE 1.07s The missing 1.08s is the pair of subqueries that label a payload hit with its tool name and its snippet. They are correlated to one turn, but they were written FTS-table-first with turn_id as a filter on the output, and tool_uses had no index on turn_id — so SQLite answered "the matching tool use of this turn" by walking all 27,315 all-time matches for `git`, twice per returned row. That cost is per returned row, so it neither shrinks with the window nor shows up in the CTE's plan. Turning each subquery around — seek tool_uses by turn_id, then probe the index for that one document — makes the work the turn's own tool uses. The two compose: before 1.19s period terms only 1.07s turn-first attribution only 0.11s both 0.003s CROSS JOIN is load-bearing. SQLite has no row estimate for an fts5 MATCH, so left to choose it puts the virtual table first and the scan comes back; the plan assertion in TestSearch_PayloadAttributionDrivesFromTheTurn fails if either the CROSS JOIN or the index goes away. A plan assertion because both shapes return identical rows and the difference is invisible on a fixture — 1.08s against 0.001s only exists at a million turns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured rather than assumed. Dropping idx_tool_uses_turn_id and keeping the CROSS JOIN turns each attribution lookup into a scan of all 264,199 tool uses per returned row: 3.0s for `git after:2026-10-01` against the 1.2s the unpinned shape costs without the index. So the index is a dependency of this query shape, not a tuning of it, and the comment says so where the shape is written. Also corrects the index's measured size in migration 010. It is 2,940 pages — 12 MB — where the comment said 5 MB; 5 MB was the file's growth, which is smaller because the FTS rebuild above it leaves free pages for the index to take. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds timestamp-derived day and month tokens to turn and tool-use FTS indexes. Search compiles date bounds into scoped FTS expressions and resolves missing bounds from archive timestamps. Database and search tests cover token indexing, date bounds, expression scoping, and query behavior. ChangesDate-Filtered Search
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Search
participant periodTermsFor
participant ArchiveDB
participant buildQuery
participant turns_fts
participant tool_uses_fts
Search->>periodTermsFor: Resolve date bounds and period terms
periodTermsFor->>ArchiveDB: Read oldest or newest turn timestamp for a missing bound
periodTermsFor-->>Search: Return compiled period terms
Search->>buildQuery: Pass period terms
buildQuery-->>Search: Return scoped FTS expressions and query arguments
Search->>turns_fts: Match turn content with period terms
Search->>tool_uses_fts: Match tool payloads with period terms
Merge Risk: ⚪ Minimal · up to The reviewed date-filtered search change is mergeable after normal checks. No demonstrated regression remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Existing access controls are preserved, but very wide date queries can bypass the intended work limit and produce oversized search expressions. The confirmed exposure is to callers already able to search the archive. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. I’m a rabbit with dates tucked under my paw, Comment |
The "why" block quoted an early, noisier run. The final figures, minimum of seven runs of the real CLI against the real archive: `git` all-time 2.04s and `git after:2026-10-01` 1.21s, which is also what the same query costs for a thirty-five-day window — the flatness is the point and it was missing from the note. Also fixes a wrong direction: the index takes its free pages from the two rebuilds below it, not above it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Interaction with #88, worked through#88 ( The period terms stay correct under #88's semanticsThe safety argument here is that a row the timestamp predicate admits always carries a token in the compiled set, because the predicate is a lexicographic comparison of text whose first ten characters are the calendar date: if a row's date prefix sorted below the bound's, the whole string would too. #88 changes the bounds but not that property.
What breaks on merge, and it is tests rather than behaviour
And the fixture's instants are UTC while #88 parses bounds as local, so on a non-UTC machine the day boundaries shift relative to each other. The fixture should pin a location explicitly once #88's reading is the real one. Nothing in |
Verified interaction with #88 — clean textual merge, three failing tests#88 (
Actually merging them and running the tests gives three failures: The failures are correct. #88 makes Nothing in the pruning is broken. Every failure is a case gaining rows it should now gain; no case lost a row. That is the outcome the period-token design predicts: #88's inclusive Two further points of composition worth recording:
What whoever merges second needs to doUpdate three expectations in CI status on this PRBuild and Lint pass; CodeRabbit passes. The three |
1 participant
Closes #80. Repairs #81 as a side effect — see "What this does to #81" below.
The goal, and whether it was met
Met. A date-filtered search's cost now tracks the window asked for rather than the size of the archive. Measured on a copy of the author's 951,548-turn / 264,199-tool-use / 5.37 GB archive:
gitwith a date filterafter:2026-10-05after:2026-10-01after:2026-09-28after:2026-09-01after:2026-08-01Today's column is flat at 1.21s from a five-day window to a thirty-five-day one — the fixed cost #80 is about. This branch's column slopes with the window: subtract the 0.02s floor (process start plus opening a 6 GB database, measured with a query that matches nothing) and the query costs 0.00s, 0.01s, 0.04s, 0.12s, 0.53s against window sizes of 0.6%, 1.8%, 9.4%, 33.8%, 100% of the archive. Cost is now roughly proportional to the window, where before it was roughly constant.
The one-day row is cheap today for a reason worth naming: it returns 1 result rather than 20, and most of today's fixed cost is per returned row, not per match. That is half of what this PR fixes and it was not in #80's diagnosis.
The queries in #80's table
"git commit"all-time"git commit" after:2026-09-28I could not reproduce #80's absolute numbers for this pair and I am not going to pretend otherwise.
"git commit"as a phrase matches 1,412 turns and 652 tool uses on the current archive — not the 2,432 and 10,429 the issue reports — so whatever was measured there was matching about sixteen times as much material as the phrase does now. The issue's signature reproduces exactly, though: 0.16s all-time against 0.14s for a window holding 1.8% of the archive is the same "the filter prunes results but not the scan" shape, just with smaller absolute numbers. Single-word queries are where it bites, so the table above usesgit, and two more for breadth:commitall-timecommit after:2026-09-28errorall-timeerror after:2026-10-01All numbers are the minimum of 7 runs of the real CLI against the real archive, warmed first. CPU time (user+sys) tracked wall clock to within 0.01s throughout, so these are not I/O artefacts; the machine carries a permanent 95%-CPU Spotlight process, which is why the minimum rather than the mean is quoted. Spread was within 10% except where noted in the thread.
Two causes, not one
#80 diagnosed the
text_hitsCTE: a UNION of every all-time match materialised into a temp B-tree before the date predicate is consulted. That is real, and the fix is the period token. But it is a third of the problem. Timed piece by piece,git after:2026-10-01:text_hitsCTE, as todaytext_hitsCTE, period-prunedThe missing second is the pair of correlated subqueries #28 added to label a payload hit with the tool that matched and to cut its snippet. They are scoped to one turn, but they were written FTS-table-first with
turn_idas a filter on the output, andtool_useshad no index onturn_id— so SQLite answered "the matching tool use of this turn" by walking all 27,315 all-time matches, twice per returned row. Per returned row, so pruning the index cannot reach it, and it does not shrink when the window does.Both fixes are needed and they compose:
git after:2026-10-01Design decisions
Granularity: day and month, with the range decomposed. Month alone keeps a year to twelve terms but is five times coarser than the window people ask for:
after:2026-09-28admits 16,687 turns where September and October together hold 89,071. Day alone makes a year 365 terms. Carrying both and spending days only on the partial month at each end covers the range exactly — a week is seven terms, a year about seventy, and the pruned set is the window rather than a rounding of it. For the measured query the compiled terms admit 885 documents where the unpruned match admits 35,067.Namespace:
ccvd20261005/ccvym202610, plus a column filter. The prefixes are so the tokens read as tokens in anEXPLAIN; they are not what prevents collision, because unicode61 tokenizes a query the same way it tokenizes content and any term a document holds is a term a user can type. What prevents collision is that every MATCH this codebase builds is now column-scoped: the caller's text against the content columns, period terms againstsearch_period. So a message containingccvym202610is found by searching for it and contributes nothing to any date filter, and a date filter cannot be answered by anything but the period column.db.SearchTurnsandSearchTurnsWithFiltersare scoped the same way — they are test-only today, but they are the other door intoturns_fts. Measured: scoping costs nothing (0.072s against 0.074s for the same CTE).Storage: a VIRTUAL generated column, plus one new FTS column. No bytes in the table, no backfill UPDATE at all, and the token cannot drift from the timestamp it describes. Sliced out of the stored text rather than read with
strftime, which is the load-bearing detail: the date filter is a lexicographic comparison of that same text, so deriving the token from the text ties the token to the comparison. If a row's date prefix sorted below the filter's bound, the whole string would too — which is the proof that the terms can only ever admit more rows than the predicate keeps, never fewer. An extra FTS column rather than appending to the indexed text, because appending would put the token insidesnippet()'s output.Both indexes.
turns_ftsas well astool_uses_fts. A UNION is only as bounded as its looser branch, and leaving one unbounded leaves the cost growing with the archive — which is the property being fixed, not the absolute number.Correctness
Not negotiable for a performance change, so it is asserted three ways.
before:-only range, and an all-time control. Identical turn ids in identical order every time, and identical tool attribution and snippet text across 595 attributed rows.TestSearch_DateFilterReturnsTheSameRowsAtPeriodBoundariesseeds a turn at 23:59:59.999999999 on the last day of a month and another 1 ns past the following midnight, then runs eight queries across that edge, including a payload-only match and a window that compiles to a single month term. It was written against the unwired code and passed there first, which is what makes it a control rather than a tautology: the rows are the same before and after.WHERE timestamp > ?is what decides, and it is what trims the partial first and last day to the hour. A timestamp the period column cannot read as a date gets accvymxsentinel that every compiled term set includes, so such a row stays a candidate and the predicate decides it exactly as before — a missing token would be a wrong answer, where this change is only meant to buy a faster one.How the pruning is proved, separately from correctness
A test that asserts "the right rows come back" passes before and after, so it proves nothing about pruning. Two tests do that job:
TestSearch_DateFilterPrunesInsideTheIndexbuilds a fixture of 246 matching turns, 243 of them outside the window, takes the MATCH expression the searcher actually binds, and counts the documents the index yields for it. Unfiltered: 246. Filtered: 3. The control is in the same test and the same fixture, so the two counts are the pruning.TestSearch_PayloadAttributionDrivesFromTheTurnasserts the query plan, because the attribution defect is invisible in everything but the wall clock — both shapes return identical rows, and 1.08s against 0.001s only exists at a million turns. The assertion is thatidx_tool_uses_turn_idappears twice, once per subquery. It fails against the old shape, and it fails if either the index or theCROSS JOINthat pins the join order is removed.CROSS JOINis load-bearing. SQLite has no row estimate for an fts5 MATCH, so left to choose it puts the virtual table first and the scan comes back: the SQLite thatmodernc.org/sqlitebundles happens to pick the index for the old shape too once it exists, but system SQLite 3.51 keeps the scan even with the index present (1.19s, plan verified). Pinning the order makes the plan a property of the query rather than of the planner's mood. It also makes the index a hard dependency rather than an optimisation — pinned order with no index to seek is a scan of all 264,199 tool uses per returned row, 3.0s, worse than today.What this does to #81
It repairs it, and that is announced rather than quiet because it changes the issue's disposition.
Migration 010 cannot alter an FTS5 column list, so it drops and recreates both indexes and derives them from the content tables. On the author's archive
turns_ftsheld 1,004,353 documents for 951,548 turns — 52,805 orphans (#81 counted 29,268 when it was filed; the archive has grown since). After the migration:turns_fts_docsizeholds exactly 951,548 rows,tool_uses_fts_docsizeexactly 264,199. Zero orphans by theNOT EXISTScheck in both.integrity-checkwith rank 1 passes on both — 15.8s forturns_fts, 4.3s fortool_uses_fts. That is the check Live archive has 29,268 orphaned turns_fts entries; strict integrity-check reports malformed #81 reports as returning "database disk image is malformed", and it is the only check that can see this:COUNT(*)on an external-content table resolves through the content table and agrees with itself however far the index has drifted, and plainPRAGMA integrity_checkreturns clean too.So #81 can be closed by applying this migration. What it does not do is explain how the orphans got there or prevent a recurrence — that was the pre-
recursive_triggersera, already fixed, and this is a repair of the damage rather than of the cause.Migration cost, measured
Timed end to end on a copy of the author's 5.37 GB archive, going from schema 9 to schema 10:
rebuildeach.idx_tool_uses_turn_idis 0.3s to build and 2,940 pages — 12 MB — of which only 4.8 MB shows up as file growth, the rest coming out of the free list the rebuild left.ADD COLUMNs are absorbed by the migrator's existing duplicate-column tolerance, and everything else isDROP ... IF EXISTSplus arebuildthat re-derives rather than duplicating.Verification
Out of scope, found on the way
Four things this PR deliberately does not fix:
Searchfail outright —sql: Scan error on column "timestamp": unsupported Scan, storing driver.Value type string into type *time.Time— so the whole query errors rather than the row being skipped. Pre-existing, unrelated to this change, and the reason the sentinel test asserts on the index rather than throughSearch.before:is exclusive at midnight of the named day, sobefore:2026-09-30excludes everything that happened on the 30th. Long-standing, surprising, and the period terms deliberately agree with the predicate rather than with what the operator sounds like.tool_usesholds 6,378 rows whoseturn_idnames no turn. They can never be returned (the search joins throughturns), so they are invisible rather than wrong, but they are 2.4% of the table and they are indexed.after:todaymeans "after local midnight, compared as a string against UTC timestamps", which is off by the offset. Pre-existing; the period terms are derived from the same rendering so they agree with it exactly rather than papering over it.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit