feat(indexer): push result cap and ordering into KV tx search (PLT-748)#3708
feat(indexer): push result cap and ordering into KV tx search (PLT-748)#3708amir-deris wants to merge 4 commits into
Conversation
amir-deris
changed the title
PR SummaryMedium Risk Overview KV tx indexer: Adds a fast path ( RPC Shared helpers: Tests add Reviewed by Cursor Bugbot for commit 275cbac. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3708 +/- ##
==========================================
- Coverage 59.33% 58.39% -0.94%
==========================================
Files 2274 2188 -86
Lines 188703 178997 -9706
==========================================
- Hits 111966 104530 -7436
+ Misses 66672 65191 -1481
+ Partials 10065 9276 -789
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-tested refactor that pushes the result cap and (height, index) ordering into the KV tx indexer (fast path for equality/tx.height-range queries, materialize-then-bound fallback otherwise) and hoists the shared bounding helpers into the indexer package. No blocking issues found; a few non-blocking notes, including an assessment of Codex's deprecated-key concern.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's only finding (regression on deprecated slash-delimited rows like
sender/addr1/height/indexforsender = 'addr1') is most likely NOT a regression: both the oldmatchpath and the newsearchBoundedderive the scan prefix from the sameprefixFromCompositeKeyAndValue(tag, value), which is an orderedcode encoding (sender\x00\x01addr1\x00\x01). The manually-written legacy raw-byte keysender/addr1/...diverges at byte 6 (0x00vs'/'), so it never shared the orderedcode prefix and was never visited by either the old or new iterator — the query returned empty before this PR too. The real issue is thatTestTxSearchDeprecatedIndexingasserts vacuously (it ranges overresultswithout arequire.Len), so it cannot distinguish empty from non-empty and never actually exercised deprecated support. Recommend strengthening that test with an explicit length assertion to remove the ambiguity Codex flagged. - Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - RPC
TxSearchre-sorts and re-caps results that the KV indexer has already ordered and bounded. This is intentional and documented as a safety net for sinks that ignore the limit, so it's fine, though for the KV fast path it is redundant work. - Behavioral tightening (acceptable/likely an improvement): invalid
order_bynow errors up front before the sink loop, whereas previously the error was produced inside the per-sink sort switch. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| height, index, err := parseHeightIndexFromKey(it.Key()) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
[nit] Candidates whose secondary key fails to parse are silently skipped (continue). This is the one behavioral difference from the old match, which collected every value under the driver prefix regardless of key shape. In practice all real keys are orderedcode-encoded via secondaryKey, and legacy raw/slash-delimited keys do not share the orderedcode driver prefix, so this should be unreachable for well-formed stores — but if any foreign key ever did land under a matched prefix, the fast path would drop it whereas the old path returned it. Consider at least a debug log here so silent skips are observable.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped, well-tested follow-up that bounds the KV tx-search scan path (fast point-probe path + materialize-then-cap fallback) and de-duplicates shared bounding helpers into the indexer package. The logic is correct and the fast/fallback paths are consistent; no blocking issues found, only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review (cursor-review.md) produced no output; the Codex review (codex-review.md) reported no material findings but noted it could not run the tests because the sandbox couldn't fetch the Go 1.25.6 toolchain. This review's correctness assessment is therefore based on static analysis, not a local test run.
- Test coverage suggestion: TestTxSearchBounded's "bounded cap matches unbounded prefix" invariant compares each query against its own unbounded run, but there is no direct cross-check that the fast path and the fallback path return identical ordered results for a query where both could in principle apply. Given HeightInRange mirrors matchRange's comparisons this is low risk, but an explicit fast-vs-fallback equivalence assertion would harden against future drift between the two paths.
- The searchBounded
seenmap is defensive — under the current single-driver-prefix scan each tx yields exactly one key, so duplicates cannot occur unless a tx re-emits the identical indexed attribute (deduped at the DB key level anyway). It is harmless but worth a one-line comment clarifying it guards a theoretical case rather than an observed one. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // sort results (must be done before cap and pagination) | ||
| switch req.OrderBy { | ||
| case DescendingOrder, "": | ||
| if orderDesc { |
There was a problem hiding this comment.
[nit] Minor: the KV indexer now already returns results ordered and capped to MaxTxSearchResults, so this RPC-layer re-sort is redundant work for the KV sink. It's intentional and documented as a safety net for sinks that ignore the limit/ordering, and the data set is bounded by MaxTxSearchResults, so the cost is negligible — noting only for awareness. No change required.
There was a problem hiding this comment.
A well-structured, well-tested follow-up that bounds the KV tx-search scan path (fast point-probeable path + materialize-then-cap fallback) and de-duplicates shared bounding helpers into the indexer package. I found no correctness, security, or performance blockers; only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor second-opinion pass produced no output. Codex (codex-review.md) reported no material findings (it could not rungo testbecause the sandbox lacked the Go 1.25.6 toolchain).- Minor behavior change in
TxSearch: an invalidorder_byis now rejected up front, before the sink loop, instead of only inside the KV-sink branch. This is strictly better (consistent withBlockSearch) but means a malformedorder_bynow errors even on nodes without a KV sink. Worth a mention only; no action needed. - The RPC layer still sorts results after the indexer already sorts and caps them. This is intentional (documented as a safety net for sinks that ignore the limit/ordering) and cheap since the set is already bounded — no change requested.
- Verified the moved helpers (
BoundedCap,HeightInRange,PrefixUpperBound,maxBoundedPrealloc) are byte-identical to the formerblock/kvlocals, andblock/kv/util.gono longer references theindexerpackage after dropping the import — no dangling import. Could not rungo build/go testhere (sandbox denied), so a CI green run is the final confirmation.
1 participant
Summary
Follow-up to #3689 (PLT-748), which pushed the result cap and ordering down
into the KV block indexer but left the tx scan path unbounded, marked
TODO(PLT-748).TxSearchstill fetched the entire match set, sorted it, andonly then applied
MaxTxSearchResultsat the RPC layer — so a broad tx querymaterialized and sorted far more results than the caller would ever see.
This PR bounds the tx scan path the same way the block indexer is bounded, and
de-duplicates the shared bounding helpers into the
indexerpackage so bothindexers reuse one implementation.
What changed
tx/kv) — bounded search:planBounded/searchBounded): eligible when every conditionis an equality (point-probeable) or a
tx.heightrange (evaluable from thecandidate height), with at least one equality to drive the scan. It scans the
driver equality's secondary-index prefix
(tag, value)— which orders by(height, index)— inorder_byorder, point-probes the remainingequalities per candidate via
store.Has(secondaryKey(...)), evaluatesheight ranges from the candidate height, dedups by hash, and stops at
Limit. Memory is bounded by results kept, not by total match cardinality.intersect+collectBounded): queries withCONTAINS/MATCHES/EXISTS, non-height ranges, or only atx.heightrange cannot drive an in-order point-probeable scan — the
tx.heightsecondary index stores the height as a decimal string, so its key order is
not numeric. These materialize the intersection as before, then order by
(height, index)and cap.TxSearch: validatesorder_byup front, pushes the cap and orderinginto the indexer, and keeps the post-sort cap as a safety net for sinks that
ignore the limit (mirrors
BlockSearch). Removes theTODO(PLT-748).BoundedCap,HeightInRange, andPrefixUpperBound(plus themaxBoundedPreallocconst) fromblock/kvintothe
indexerpackage as exported helpers, and updated bothblock/kvandtx/kvto use them.parseHeightIndexFromKeystays tx-local (tx keys carry anindexcomponent; block keys do not).Tests
TestTxSearchBoundedcovers the fast path (equality driver, multi-equalityprobe, equality +
tx.heightrange,tx.heightequality driver) and thefallback path (
tx.height-range-only,CONTAINS), across asc/desc andvarious limits. It also asserts a cancelled context returns partial results
without error, and that the bounded top-N equals the first N of the same query
run unbounded — guarding against divergence between the fast and fallback
paths.
TestTxSearch*suites are unchanged (behavior for the zero-value,unbounded opts is preserved).
Known limitation / follow-up (PLT-786)
The fallback path still fully materializes its intersection before the cap is
applied — this covers
CONTAINS/MATCHES/EXISTS, non-height value ranges, andtx.height-range-only queries. These query shapes can't drive an in-order,point-probeable scan in the current index format: the
tx.heightsecondaryindex stores the height as a decimal string (
orderedcode(TxHeightKey, "N", …)),so its key order is lexicographic, not numeric, and there is no equality to drive
off. The RPC layer still caps the returned set, so this is a transient
memory/CPU bound (scales with match cardinality), not an unbounded result.
Bounding these internally needs a height-ordered secondary index
(
orderedcode(compositeKey, height, value, index)), which is a storage-formatchange (write amplification + reindex/migration on upgrade) and out of scope
here. That work is tracked in PLT-786, which also makes numeric
tx.heightranges seekable for free. This PR intentionally bounds only the equality /
tx.height-point cases that the existing index can serve in order.