perf(sdk): push ledger Query filters into SQL + adopt prepare_cached#400
Merged
Conversation
Closes #324. The ledger query verbs (`query_turns`, `query_compactions`, `query_relationships`, `query_tool_result_events`, `query_user_turns`) used to load every row of the target table into Rust and then filter in memory. Move the `since` / `until` / `session_id` / `source` / `project` predicates into a generated SQL `WHERE` clause so the existing indexes (`idx_turns_ts`, `idx_turns_session`, etc.) actually do work, and switch hot SELECTs to `prepare_cached` so `burn ingest --watch`'s once-a-second read no longer pays full prepare/teardown. Folded-stamp enrichment is the only Query predicate that still runs in Rust — stamps live in their own table — so `query_turns` keeps a post-filter for that one case. Adds three regression tests pinning the new SQL semantics: - combined since/source/project filters on `turns` - relationships `session_id` matching either endpoint - tool_result_events with null `ts` surviving a `since` bound
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/relayburn-sdk/src/ledger/tests.rs (1)
952-962: ⚡ Quick winMake the
projectregression actually distinguishprojectfromproject_key.This fixture currently sets both columns to
"burn"on every matching row, so the test still passes if the SQL regresses to checking only one side of(project = ? OR project_key = ?). Seed one row viaprojectonly and another viaproject_keyonly.Suggested test tightening
- t_a.project = Some("burn".into()); - t_a.project_key = Some("burn".into()); + t_a.project = Some("burn".into()); + t_a.project_key = None; let mut t_b = make_turn("s1", "m2", "2025-01-02T00:00:00Z", 20); - t_b.project = Some("burn".into()); - t_b.project_key = Some("burn".into()); + t_b.project = None; + t_b.project_key = Some("burn".into());Also applies to: 997-1005
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/relayburn-sdk/src/ledger/tests.rs` around lines 952 - 962, The test fixture currently sets both t_a.project and t_a.project_key (and likewise t_b) to the same value so it doesn't catch regressions that only check one column; change the seeds so one row has only project set (e.g., set t_a.project = Some("burn") and leave t_a.project_key = None) and another row has only project_key set (e.g., set t_b.project_key = Some("burn") and leave t_b.project = None), then call l.append_turns(&[t_a, t_b, t_c]).unwrap(); do the same adjustment for the similar block around the t_a/t_b/t_c at the other occurrence referenced (lines ~997-1005) to ensure the SQL must check both project OR project_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/relayburn-sdk/src/ledger/tests.rs`:
- Around line 952-962: The test fixture currently sets both t_a.project and
t_a.project_key (and likewise t_b) to the same value so it doesn't catch
regressions that only check one column; change the seeds so one row has only
project set (e.g., set t_a.project = Some("burn") and leave t_a.project_key =
None) and another row has only project_key set (e.g., set t_b.project_key =
Some("burn") and leave t_b.project = None), then call l.append_turns(&[t_a, t_b,
t_c]).unwrap(); do the same adjustment for the similar block around the
t_a/t_b/t_c at the other occurrence referenced (lines ~997-1005) to ensure the
SQL must check both project OR project_key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ec8fcc5-3adf-49a2-9c4d-68a7735d9010
📒 Files selected for processing (3)
CHANGELOG.mdcrates/relayburn-sdk/src/ledger/reader.rscrates/relayburn-sdk/src/ledger/tests.rs
CodeRabbit nit on #400: the original fixture seeded `project` and `project_key` to the same value on every matching row, so the test would pass even if the SQL regressed to checking only one of the two columns. Seed one row via `project` only and another via `project_key` only so a single-column SQL bug fails the test.
…-issue-mIHWj # Conflicts: # CHANGELOG.md
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
…-issue-mIHWj # Conflicts: # CHANGELOG.md Co-authored-by: willwashburn <957608+willwashburn@users.noreply.github.com>
Contributor
Resolved and merged latest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #324.
Summary
crates/relayburn-sdk/src/ledger/reader.rswas loading every row of the target table into aVec<String>and then filtering in Rust. Pure SQL predicates (since,until,session_id,source,project) now ride into a generatedWHEREclause so the existing indexes (idx_turns_ts,idx_turns_session, etc.) actually do work, and the hot SELECTs useConnection::prepare_cachedsoburn ingest --watch's once-a-secondingest_allread no longer pays full prepare/teardown.build_select_sqlhelper that emits a stable SQL string per filter combination —prepare_cachedreuses the compiled statement.ts, OR-on-related_session_id, project columns) is encoded in a smallTableFiltersstruct so the helper stays declarative.Querypredicate still evaluated in Rust — stamps live in their own table — soquery_turnskeeps a post-filter for that one case.collect_stamps,list_user_turn_session_ids, andraw_record_jsonsalso moved toprepare_cached.Test plan
cargo test --workspace(647 tests inrelayburn-sdk+ 89 across cli/ingest helpers, all green)since/source/projectfilters onturnssession_idmatching either endpoint (session_idORrelated_session_id)tool_result_eventsrows with nulltssurviving asinceboundcargo build --workspacehttps://claude.ai/code/session_01MzV5fYE85XC1JwNn6deQ8R
Generated by Claude Code