perf(core): index sparse failure results by session - #145
perf(core): index sparse failure results by session#145kongtou20070406 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new query-plan regression test’s source filter predicate diverges from the production failures() semantics (COALESCE(s.source,'claude')), which can make the test assert the wrong plan/behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a partial SQLite index to accelerate the failures() query path by restricting scans to the sparse subset of failure-like tool_results rows, while keeping query semantics stable and guarding the query plan via regression testing.
Changes:
- Add a partial index on
tool_results(session_id)for rows matching the failure predicate (is_error = 1 OR content LIKE 'Exit code %'). - Add an
EXPLAIN QUERY PLANregression test to assert the new index is used for provider-filtered failure queries. - Update the provider schema stability digest to reflect the schema change.
File summaries
| File | Description |
|---|---|
| tests/provider-schema-stability.test.mjs | Updates the pinned schema hash for the new index. |
| tests/db-schema.test.mjs | Adds a query-plan regression test validating use of the partial failure index. |
| packages/core/src/schema.sql | Introduces the new partial index idx_tr_failure_session to speed up failure scans. |
Review details
- Files reviewed: 3/3 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.
| FROM tool_results tr | ||
| LEFT JOIN sessions s ON s.id=tr.session_id | ||
| WHERE (tr.is_error = 1 OR tr.content LIKE 'Exit code %') | ||
| AND s.source = ? | ||
| `).all('codex'); |
There was a problem hiding this comment.
Fixed in e47e2bf: the test now copies production's COALESCE(s.source, 'claude') predicate and all production joins/visibility/order/limit clauses. I retained LEFT JOIN intentionally because production must preserve the NULL-source => claude compatibility behavior; an INNER JOIN would drop missing session rows and would not mirror failures(). The focused EXPLAIN regression passes.
|
Fixed in the latest commit: the query-plan regression now mirrors the production joins, visibility predicates, COALESCE(source, 'claude') behavior, ordering, and limit. The focused plan test passes. Please re-review. |
Summary
failures()Evidence
On a recoverable 3.33 GB real-history snapshot (233,016 tool results; 1,029 failures), 20 repeated provider-filtered calls changed as follows:
Every before/after result hash matched. One-time index creation on the existing DB was 4.683 s. A fresh 1.94 GB Codex build showed identical row counts and semantic digest; one baseline/candidate pair was 10.69 s / 10.12 s, so no fresh-build regression was observed.
Checks