Add batched fetch_order_trades variant (IN clause)#2711
Conversation
Adds `build_fetch_order_trades_batch_stmt()` and a `fetch_order_trades_batch()` wrapper that fetch trades for many order hashes in a single `WHERE order_hash IN (...)` query, eliminating the N+1 query (and per-query connection) overhead described in #2543 when trades for several orders are requested together. The order-hash predicate in query.sql is now a `/*ORDER_HASH_CLAUSE*/` marker: the single-order builder fills it with `= ?3` via `bind_param_clause` (identical placeholder/param ordering to before), and the batch builder fills it with `IN ({list})` via the proven `bind_list_clause` pattern already used by fetch_orders. The wrapper short-circuits an empty input to an empty result without touching the DB. No bytecode or deploy changes. Fixes #2543 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 21 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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. Comment |
The single-order `fetch_order_trades` wrapper now delegates to
`fetch_order_trades_batch` with a one-element `&[order_hash]` slice, so
`build_fetch_order_trades_batch_stmt` (the `WHERE order_hash IN (...)`
builder) is the one underlying query path for the single-order trade fetch
used in production by `LocalDbOrders::trades_list`. The batch logic is now
exercised on every single-order trade fetch with zero behavior change: for a
one-element list SQLite's `IN (?3)` is equivalent to the old `= ?3`, the
SELECT/ORDER BY/time-window binding are identical, and the result is already
filtered to the requested hash so it is returned as-is.
The now-redundant single-order SQL builder `build_fetch_order_trades_stmt`
(and its `ORDER_HASH_SINGLE_BODY = "= {param}"` constant) had no remaining
production or test references, so they are removed. The two builder unit
tests that only covered it are dropped; their single-hash + time-window
coverage is preserved by the new `batch_single_hash_with_time_filters_binds_window`
test and the strengthened `batch_single_hash_renders_single_placeholder_in_clause`.
Adds a discriminating wasm test
`single_order_wrapper_returns_only_that_orders_trades_via_batch`: a callback
that mimics the DB's `order_hash IN (...)` filter (returning only rows whose
hash is actually bound into the query) proves the single-order wrapper returns
exactly the requested order's trades via the batch path, identical to calling
the batch fn with a one-element list, and never the wrong order's rows, both
orders' rows, or an empty result. This module's wasm tests previously ran in
no CI job, so they are gated on `browser-tests` and added to
wasm-browser-test.yaml so the batch path is genuinely verified in CI.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the implicit "empty slice means all trades" footgun in build_fetch_order_trades_batch_stmt with an explicit OrderHashFilter enum: All drops the order-hash predicate (every order), while These(&[..]) filters to exactly those hashes. An empty These now means *none* — it emits a constant-false 'AND 1=0' predicate (SQLite rejects 'IN ()') so empty filters return zero rows instead of silently fetching all. All and These(empty) are explicit opposites. The fetch_order_trades_batch empty-input short-circuit is retained but re-documented as a perf optimization (skip the DB round-trip) rather than a correctness guard, since These(&[]) would already return no rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviewed 029b349: explicit OrderHashFilter { All, These(&[B256]) } replacing implicit empty-means-all (All drops the clause; These([]) emits AND 1=0 = explicit none, never all); mutation-validated, 1077 tests green, rebased on green main, all checks green. LGTM. |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
1 participant
What
Adds a batched variant of
fetch_order_tradesso trades for many order hashes can be fetched in a single SQL query instead of N separate ones.build_fetch_order_trades_batch_stmt(raindex_id, &[B256], start_ts, end_ts)— emits the same query as the single-order builder but withWHERE order_hash IN (...).fetch_order_trades_batch(...)wrapper inraindex_client— short-circuits an empty input to an empty result without touching the DB.query.sql: the order-hash predicate is now a/*ORDER_HASH_CLAUSE*/marker. The single-order builder fills it with= ?3viabind_param_clause(identical placeholder/param ordering to before, so the existing path is unchanged); the batch builder fills it withIN ({list})via the provenbind_list_clausepattern already used byfetch_orders.Why
Per #2543, there's no batch variant of
fetch_order_trades, so callers must loop over order hashes one at a time. A singleWHERE order_hash IN (...)query eliminates redundant CTE evaluations and per-query connection overhead (the N+1 pattern described in the issue). This is the batch half of the issue; it composes with connection pooling (#2708) as an orthogonal optimization. The existingidx_derived_trades_order_hash_timeindex covers the predicate.No bytecode or deploy changes.
Verification
cargo test -p raindex_common --lib local_db::query::fetch_order_trades— 8 passed (includes new batch builder tests: IN-clause rendering + time filters, single-hash, empty-list clause removal).cargo test -p raindex_common --lib local_db::query— 195 passed (no regression in the single-order path or sibling query builders).cargo check -p raindex_common --target wasm32-unknown-unknown --tests— clean (new wasm wrapper tests compile).Fixes #2543
🤖 Generated with Claude Code