Repository navigation
feat(pipeline-io): arena sort steps + N+2 coordination pipeline - #474
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
WalkthroughReplaces the legacy monolithic sort path with arena-backed ingest, split spill processing, tunable decompression, generic merge framing, detached writer support, and updated parity tests plus benchmark wiring. ChangesSort pipeline redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e4354d1 to
5832bac
Compare
ec74763 to
e459eca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/fgumi-pipeline-io/benches/serial_ingest.rs`:
- Line 1: The bench crate root in serial_ingest is missing the crate-level
unsafe ban, so add the same crate-root deny attribute used by sibling benches to
keep parity with the coding guideline. Update the top of serial_ingest.rs,
alongside the existing bench crate declaration, to include the unsafe-code
denial at crate root so this benchmark crate is covered by the same policy.
In `@crates/fgumi-pipeline-io/src/sort/compress_spill.rs`:
- Around line 138-139: The intra-doc links in the module and function comments
are stale because fgumi_sort::write_sorted_chunk is no longer the target. Update
the documentation around compress_spill and the dispatch helper to reference
write_sorted_chunk_inmem and write_spill instead, using the existing doc
comments near the MemoryChunkErased dispatch logic so the links resolve
correctly.
In `@crates/fgumi-pipeline-io/src/sort/sort_buffer.rs`:
- Around line 318-334: The SortBuffer batch ingestion path is accumulating
multiple spill events in pending during a single RecordBatch, which lets memory
grow beyond a single chunk. Update the push loop in SortBuffer::push_batch
(around the sorter.push/take_sorted_chunk flow) so staged spills are drained or
yielded before continuing to ingest the rest of the batch, or otherwise bound
the upstream RecordBatch size, using the existing SortChunkEvent::Spill and
next_seq mechanics to keep retained memory near one memory_limit chunk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c20d957-21fe-44ba-a1cb-09815e522802
📒 Files selected for processing (23)
crates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/benches/serial_ingest.rscrates/fgumi-pipeline-io/src/boundaries.rscrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/arena_ingest.rscrates/fgumi-pipeline-io/src/sort/compress_spill.rscrates/fgumi-pipeline-io/src/sort/compress_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/sort_buffer.rscrates/fgumi-pipeline-io/src/sort/spill_compress.rscrates/fgumi-pipeline-io/src/sort/spill_compress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_gather.rscrates/fgumi-pipeline-io/src/sort/spill_gather/tests.rscrates/fgumi-pipeline-io/src/sort/spill_write.rscrates/fgumi-pipeline-io/src/sort/spill_write/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rs
💤 Files with no reviewable changes (2)
- crates/fgumi-pipeline-io/src/sort/and_spill/tests.rs
- crates/fgumi-pipeline-io/src/sort/and_spill.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
5832bac to
2d1e439
Compare
e459eca to
9a11221
Compare
2d1e439 to
bf8e3d7
Compare
9a11221 to
41d6ad5
Compare
bf8e3d7 to
952ead0
Compare
41d6ad5 to
083a0de
Compare
083a0de to
dbc363a
Compare
952ead0 to
906ae33
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/fgumi-pipeline-io/src/sort/sort_buffer.rs (1)
303-341: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
pendingcan still stack multiple sealed chunks within oneRecordBatch.
ingest_one_batchnever stops oncepush()reportsbuffer_full; it keeps draining the rest ofbatch.iter_record_bytes(), sealing+staging a newSpillevent into the unboundedself.pendingVecDequeeach time. If a singleRecordBatchis large relative tomemory_limit, this can stack several full sorted chunks inpendingbeforetry_rungets a chance to callemit_pending, contradicting the doc's "holding at most ~one spill chunk plus the live buffer" (lines 14-17). A near-identical pattern was flagged as a major/stability issue on a prior revision and marked resolved via boundingRecordBatchupstream — worth confirming that bound still holds here, since simplybreak-ing out of this loop would silently drop the batch's unconsumed records rather than fix the memory bound.Please confirm
RecordBatchbyte/record caps stay well undermemory_limitso at most one seal fires peringest_one_batchcall in practice.🔎 Verification script
#!/bin/bash # Locate RecordBatch's size/byte cap relative to memory_limit rg -nP --type=rust -C4 'struct RecordBatch\b|impl RecordBatch\b' crates/fgumi-pipeline-io/src/types.rs rg -nP --type=rust -C4 'RecordBatch::(new|with_capacity)|max_bytes|batch_bytes|target_batch' crates/fgumi-pipeline-io/src🤖 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/fgumi-pipeline-io/src/sort/sort_buffer.rs` around lines 303 - 341, `ingest_one_batch` in `SortBuffer` can enqueue multiple `SortChunkEvent::Spill` entries in `pending` if a single `RecordBatch` is large, so verify the upstream `RecordBatch` sizing/caps keep batches safely below `memory_limit` and ensure the documented “at most ~one spill chunk plus the live buffer” invariant still holds. Check the `RecordBatch` construction path and any byte/record limits used by the pipeline, and if the cap is not guaranteed then adjust the batch-bound logic rather than changing `ingest_one_batch` to stop early and drop remaining records.
🤖 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.
Duplicate comments:
In `@crates/fgumi-pipeline-io/src/sort/sort_buffer.rs`:
- Around line 303-341: `ingest_one_batch` in `SortBuffer` can enqueue multiple
`SortChunkEvent::Spill` entries in `pending` if a single `RecordBatch` is large,
so verify the upstream `RecordBatch` sizing/caps keep batches safely below
`memory_limit` and ensure the documented “at most ~one spill chunk plus the live
buffer” invariant still holds. Check the `RecordBatch` construction path and any
byte/record limits used by the pipeline, and if the cap is not guaranteed then
adjust the batch-bound logic rather than changing `ingest_one_batch` to stop
early and drop remaining records.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb08f119-7109-4ca9-bcb1-a48867ff463b
📒 Files selected for processing (25)
CLAUDE.mdcrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/benches/serial_ingest.rscrates/fgumi-pipeline-io/src/boundaries.rscrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/arena_ingest.rscrates/fgumi-pipeline-io/src/sort/compress_spill.rscrates/fgumi-pipeline-io/src/sort/compress_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/sort_buffer.rscrates/fgumi-pipeline-io/src/sort/spill_compress.rscrates/fgumi-pipeline-io/src/sort/spill_compress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_gather.rscrates/fgumi-pipeline-io/src/sort/spill_gather/tests.rscrates/fgumi-pipeline-io/src/sort/spill_write.rscrates/fgumi-pipeline-io/src/sort/spill_write/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rs
💤 Files with no reviewable changes (2)
- crates/fgumi-pipeline-io/src/sort/and_spill/tests.rs
- crates/fgumi-pipeline-io/src/sort/and_spill.rs
36bed4c to
65ea7ca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/fgumi-pipeline-io/src/sort/tests.rs (1)
2-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winModule doc still references the retired
SortAndSpillchain.This line still reads "(
SortAndSpill→SortSpillDecompress→SortMerge)." This is the exact text a previous review flagged as stale (noted as addressed in an earlier commit), but it's back/still present here — it doesn't mentionSortBuffer/CompressSpill, which per the code (Line 417-419) is the actual production chain now.📝 Suggested doc update
-//! (`SortAndSpill` → `SortSpillDecompress` → `SortMerge`). +//! (`SortBuffer` → `CompressSpill` → `SortSpillDecompress` → `SortMerge`).🤖 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/fgumi-pipeline-io/src/sort/tests.rs` around lines 2 - 5, Update the module-level docs in sort/tests.rs to reflect the current production pipeline instead of the retired `SortAndSpill` chain. Specifically, revise the comment near the top of the file that mentions `SortAndSpill` → `SortSpillDecompress` → `SortMerge` so it matches the actual chain used by `RawExternalSorter`/the streaming sort flow, including `SortBuffer` and `CompressSpill`. Keep the wording aligned with the existing parity-oracle note around `sort_via_legacy` so the docs accurately describe the current behavior.
🤖 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.
Inline comments:
In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 189-203: The `drive_sort_buffer_pipeline` doc comment is stale and
still says the harness is “Coordinate-only,” which conflicts with
`sort_buffer_chain_matches_legacy_all_orders` using `TemplateCoordinate` and
`Queryname` via the same helper. Update the comment on
`drive_sort_buffer_pipeline` to describe the full set of supported sort orders
or remove the coordinate-only claim so the documentation matches the test matrix
and the current `SortBuffer` behavior.
---
Duplicate comments:
In `@crates/fgumi-pipeline-io/src/sort/tests.rs`:
- Around line 2-5: Update the module-level docs in sort/tests.rs to reflect the
current production pipeline instead of the retired `SortAndSpill` chain.
Specifically, revise the comment near the top of the file that mentions
`SortAndSpill` → `SortSpillDecompress` → `SortMerge` so it matches the actual
chain used by `RawExternalSorter`/the streaming sort flow, including
`SortBuffer` and `CompressSpill`. Keep the wording aligned with the existing
parity-oracle note around `sort_via_legacy` so the docs accurately describe the
current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 590fc81d-ee93-4d8e-b4d9-18f4341bb9d2
📒 Files selected for processing (25)
CLAUDE.mdcrates/fgumi-pipeline-io/Cargo.tomlcrates/fgumi-pipeline-io/benches/serial_ingest.rscrates/fgumi-pipeline-io/src/boundaries.rscrates/fgumi-pipeline-io/src/lib.rscrates/fgumi-pipeline-io/src/sink/write_bgzf.rscrates/fgumi-pipeline-io/src/sort/and_spill.rscrates/fgumi-pipeline-io/src/sort/and_spill/tests.rscrates/fgumi-pipeline-io/src/sort/arena_ingest.rscrates/fgumi-pipeline-io/src/sort/compress_spill.rscrates/fgumi-pipeline-io/src/sort/compress_spill/tests.rscrates/fgumi-pipeline-io/src/sort/merge.rscrates/fgumi-pipeline-io/src/sort/merge/tests.rscrates/fgumi-pipeline-io/src/sort/mod.rscrates/fgumi-pipeline-io/src/sort/protocol.rscrates/fgumi-pipeline-io/src/sort/sort_buffer.rscrates/fgumi-pipeline-io/src/sort/spill_compress.rscrates/fgumi-pipeline-io/src/sort/spill_compress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_decompress.rscrates/fgumi-pipeline-io/src/sort/spill_decompress/tests.rscrates/fgumi-pipeline-io/src/sort/spill_gather.rscrates/fgumi-pipeline-io/src/sort/spill_gather/tests.rscrates/fgumi-pipeline-io/src/sort/spill_write.rscrates/fgumi-pipeline-io/src/sort/spill_write/tests.rscrates/fgumi-pipeline-io/src/sort/tests.rs
💤 Files with no reviewable changes (2)
- crates/fgumi-pipeline-io/src/sort/and_spill.rs
- crates/fgumi-pipeline-io/src/sort/and_spill/tests.rs
The typed-step sort pipeline over the arena engine: ReadBlocks admit, InflateToArena, FindBoundariesAndSort, SpillGather / SpillCompress / SpillWrite, and the detached SortMerge — wired for the true N+2 model (serial coordination on a dedicated driver thread, N pool workers on pure de/compression). Removes the old and_spill step. NOTE: intermediate commit of a stacked split (see commit 3).
65ea7ca to
581657a
Compare
This branch had an error being deployed
1 participant
The typed-step sort pipeline over the arena engine:
ReadBlocksadmit,InflateToArena,FindBoundariesAndSort,SpillGather/SpillCompress/SpillWrite, and the detachedSortMerge— wired for the N+2 model (serial coordination on a dedicated driver thread, N pool workers on pure de/compression). Removes the oldand_spillstep.Intermediate PR (red CI expected): see the stack note below.
About this stack
This is a 6-PR stack that unifies the fgumi sort engine onto a single arena-based engine for all four sort orders (coordinate, queryname, template-coordinate, natural), retires the legacy sorters, and adds the N+2 threading model (serial coordination on dedicated driver threads, N pool workers on pure de/compression). The work is split one commit per crate to keep each diff small and reviewable.
Merge bottom-up:
nh/sort-1-pipeline-core→feat-runallnh/sort-2-bgzf-verify→nh/sort-1-pipeline-corenh/sort-3-arena-engine→nh/sort-2-bgzf-verifynh/sort-4-pipeline-io→nh/sort-3-arena-enginenh/sort-5-cli→nh/sort-4-pipeline-ionh/sort-6-chains-wiring→nh/sort-5-cliCI note: PRs 3–5 are the atomic engine-swap block — removing the legacy sorters leaves the workspace uncompilable until the consumers land in PR 6, so their standalone CI is red by construction. Correctness and the full green build are verified at the stack tip (PR 6): the whole suite passes (2258 tests), and sort output is byte-identical across all four orders (validated against the legacy engine and against
main). Supersedes the single-squash PR #470.Summary by CodeRabbit
SortAndSpill/SortBamFilepublic API in favor of new pipeline steps.