feat(sort): add --max-temp-files to tune the spill-file consolidation limit - #643
Conversation
|
Warning Review limit reached
Next review available in: 50 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 Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe sort command adds ChangesTemporary file limit
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SortCLI
participant build_sorter
participant RawExternalSorter
SortCLI->>build_sorter: parsed max_temp_files
build_sorter->>RawExternalSorter: apply max_temp_files(n)
RawExternalSorter-->>build_sorter: temp_file_limit()
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/commands/sort.rs`:
- Around line 157-159: Update the help text for the --max-temp-files threshold
in the sort command to say consolidation occurs when spilled runs reach the
limit, replacing “exceed” with wording that matches the engine’s len >=
max_temp_files behavior.
- Around line 875-905: Expand the max_temp_files parser tests in
test_parse_max_temp_files and test_parse_max_temp_files_rejects_below_two to
cover the valid minimum value 2, negative input, and non-numeric input. Assert
that 2 parses successfully and the negative/non-numeric values return a parse
error while preserving the existing below-two cases.
🪄 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: a667f651-5010-40fd-887e-5675f6ef4313
📒 Files selected for processing (2)
crates/fgumi-sort/src/external.rssrc/lib/commands/sort.rs
nh13
left a comment
nh13
left a comment
There was a problem hiding this comment.
Minor suggestions, when you've addressed them, feel free to squash-merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #643 +/- ##
=======================================
Coverage 93.53% 93.54%
=======================================
Files 175 175
Lines 105970 106014 +44
=======================================
+ Hits 99123 99167 +44
Misses 6847 6847 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…-rolled parser Addresses review feedback on #643. Replace the custom `parse_max_temp_files` with `clap::builder::RangedU64ValueParser::<usize>`, which parses as u64, enforces the `>= 2` floor, and converts back to usize via `TryFrom`. This keeps the field `Option<usize>` (so tools that flatten `Sort` are unaffected) while letting clap own parsing, range-checking, and overflow. clap provides no ranged value parser for `usize` itself, which is why the parser was hand-rolled in the first place; going through `u64` is the idiomatic way to range-check a `usize`. The rejection messages are now accurate: an overflowing value reports "number too large to fit in target type" rather than the previous, misleading "not a non-negative integer", and a value below the floor reports "N is not in 2..". Also correct the command-overview wording (runs are consolidated once they "reach" `--max-temp-files`, not "exceed", matching the engine's `len >= max_temp_files` guard), document the flag in the performance-tuning guide's Sort section, and bump docs/LAST_SYNCED. Expand the parser tests to cover the minimum accepted value (2) and the rejected cases (0, 1, negative, non-numeric, overflow).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/commands/sort.rs (1)
900-914: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftExercise consolidation, not just the accessor.
The new test proves that
Some(n)reachestemp_file_limit(), but never creates multiple spill runs or verifies output identity. Add an end-to-end case that forces consolidation and compares default output with a small explicit limit such asSome(2); otherwise stable-order or byte-output regressions can pass unnoticed.🤖 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 `@src/lib/commands/sort.rs` around lines 900 - 914, Add an end-to-end test alongside test_build_sorter_wires_max_temp_files that uses input large enough to create multiple spill runs, runs sorting once with the default max_temp_files and once with Some(2), then compares the resulting output bytes or records for exact identity. Ensure the test exercises consolidation rather than only inspecting temp_file_limit(), while preserving the existing accessor test.
🤖 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.
Outside diff comments:
In `@src/lib/commands/sort.rs`:
- Around line 900-914: Add an end-to-end test alongside
test_build_sorter_wires_max_temp_files that uses input large enough to create
multiple spill runs, runs sorting once with the default max_temp_files and once
with Some(2), then compares the resulting output bytes or records for exact
identity. Ensure the test exercises consolidation rather than only inspecting
temp_file_limit(), while preserving the existing accessor test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62c93e9e-3a2b-4ddd-812d-50b6b3951f0c
📒 Files selected for processing (3)
docs/LAST_SYNCEDdocs/src/guide/performance-tuning.mdsrc/lib/commands/sort.rs
…-rolled parser Addresses review feedback on #643. Replace the custom `parse_max_temp_files` with `clap::builder::RangedU64ValueParser::<usize>`, which parses as u64, enforces the `>= 2` floor, and converts back to usize via `TryFrom`. This keeps the field `Option<usize>` (so tools that flatten `Sort` are unaffected) while letting clap own parsing, range-checking, and overflow. clap provides no ranged value parser for `usize` itself, which is why the parser was hand-rolled in the first place; going through `u64` is the idiomatic way to range-check a `usize`. The rejection messages are now accurate: an overflowing value reports "number too large to fit in target type" rather than the previous, misleading "not a non-negative integer", and a value below the floor reports "N is not in 2..". Also correct the command-overview wording (runs are consolidated once they "reach" `--max-temp-files`, not "exceed", matching the engine's `len >= max_temp_files` guard), document the flag in the performance-tuning guide's Sort section, and bump docs/LAST_SYNCED. Expand the parser tests to cover the minimum accepted value (2) and the rejected cases (0, 1, negative, non-numeric, overflow).
39458d5 to
e26b763
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib/commands/sort.rs`:
- Around line 337-352: Bound the max_temp_files argument in the clap value
parser so accepted values cannot exceed a safe process/system file-descriptor
limit, rather than allowing arbitrary values through range(2..). Preserve the
existing minimum of 2 and update the related parsing tests, including the
100_000 case, to verify oversized values are rejected or safely clamped
according to the established configuration 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 Plus
Run ID: 6ff44937-9ebf-409e-8315-4cd921182150
📒 Files selected for processing (4)
crates/fgumi-sort/src/external.rsdocs/LAST_SYNCEDdocs/src/guide/performance-tuning.mdsrc/lib/commands/sort.rs
| /// Maximum number of temporary spill files kept before the oldest are | ||
| /// consolidated into a single run. | ||
| /// | ||
| /// Large inputs spill many sorted runs to disk. When the number of runs | ||
| /// reaches this limit, the oldest are merged together in a single pass so | ||
| /// the final k-way merge opens fewer files at once. Raising it avoids | ||
| /// repeated consolidation passes on very large inputs (at the cost of more | ||
| /// open file descriptors during the final merge); lowering it keeps fewer | ||
| /// files open. Must be at least 2; to effectively disable consolidation, | ||
| /// pass a value larger than the number of runs you expect to spill. | ||
| /// | ||
| /// When unset, a built-in default is used (see this command's help | ||
| /// overview for the default value). | ||
| #[arg(long = "max-temp-files", value_parser = clap::builder::RangedU64ValueParser::<usize>::new().range(2..))] | ||
| pub max_temp_files: Option<usize>, | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
No upper bound on --max-temp-files risks fd exhaustion during merge.
Values are only floored at 2 (range(2..)); nothing caps against the process/system open-file limit. A moderately large value (e.g. the 100_000 case already exercised in parsing tests) that doesn't happen to disable consolidation entirely could make the final k-way merge attempt to open one reader per surviving chunk file simultaneously, which can exceed typical ulimit -n (1024–4096) and fail mid-merge. This mirrors an unresolved suggestion from a prior review round ("should we bound the maximum number of temp files? Or clamp it to the system maximum?").
🤖 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 `@src/lib/commands/sort.rs` around lines 337 - 352, Bound the max_temp_files
argument in the clap value parser so accepted values cannot exceed a safe
process/system file-descriptor limit, rather than allowing arbitrary values
through range(2..). Preserve the existing minimum of 2 and update the related
parsing tests, including the 100_000 case, to verify oversized values are
rejected or safely clamped according to the established configuration behavior.
|
Sorry for the churn on this one — I just pushed a fix for the outstanding CodeRabbit comment (added an end-to-end test that forces spill-file consolidation and asserts the output is byte-identical between the default limit and |
… limit The external sort consolidates spilled runs once their count reaches a hardcoded limit (64, matching samtools) by merging the oldest ~half into a single run. On large inputs that spill many runs this fires repeated consolidation passes whose merge loop is single-threaded, adding significant wall-time even though the final k-way merge is unaffected. Expose the limit as `--max-temp-files <N>` on `fgumi sort`, wired through `build_sorter` to the existing `RawExternalSorter::max_temp_files` builder. The flag is optional with a minimum of 2; when unset the engine default (64) applies, so behavior is byte-identical unless the user opts in. Raising it lets large sorts skip the extra consolidation passes, at the cost of more open file descriptors during the final merge. The flag deliberately declares no clap default and states no number in its help text, so tools that embed this `Sort` via `#[command(flatten)]` can apply their own default without inheriting a wrong one. Adds a `temp_file_limit()` accessor so the option-to-builder wiring can be unit tested.
…-rolled parser Addresses review feedback on #643. Replace the custom `parse_max_temp_files` with `clap::builder::RangedU64ValueParser::<usize>`, which parses as u64, enforces the `>= 2` floor, and converts back to usize via `TryFrom`. This keeps the field `Option<usize>` (so tools that flatten `Sort` are unaffected) while letting clap own parsing, range-checking, and overflow. clap provides no ranged value parser for `usize` itself, which is why the parser was hand-rolled in the first place; going through `u64` is the idiomatic way to range-check a `usize`. The rejection messages are now accurate: an overflowing value reports "number too large to fit in target type" rather than the previous, misleading "not a non-negative integer", and a value below the floor reports "N is not in 2..". Also correct the command-overview wording (runs are consolidated once they "reach" `--max-temp-files`, not "exceed", matching the engine's `len >= max_temp_files` guard), document the flag in the performance-tuning guide's Sort section, and bump docs/LAST_SYNCED. Expand the parser tests to cover the minimum accepted value (2) and the rejected cases (0, 1, negative, non-numeric, overflow).
e26b763 to
da65744
Compare
This branch was previously deployed
2 participants
What
Adds
--max-temp-files <N>tofgumi sort, exposing the previously-hardcoded temp-file consolidation limit (DEFAULT_MAX_TEMP_FILES = 64).usize, minimum 2 (values< 2are rejected — a merge needs at least two inputs). For effectively unlimited, pass a large value.build_sorterto the existingRawExternalSorter::max_temp_filesbuilder, following the sameOption-override pattern as--sort-threads/--merge-threads.Why
The external sort spills sorted runs to disk; once the number of runs reaches the limit, the oldest ~half are consolidated into a single run so the final k-way merge opens fewer files. That consolidation merge is single-threaded, so on large inputs it adds real wall-time. Sorting a 1.29 B-read WGS BAM with defaults (≈6 GiB budget → ~93 spilled runs) triggered one or two consolidation folds costing roughly 15–38% of total wall-clock — overhead a higher limit eliminates. Concurrency during the final merge is bounded by
--threads, not the file count, so raising the limit mainly trades open file descriptors for fewer consolidation passes.Notes for embedders
The flag declares no clap
default_valueand its help text states no number, so a tool that embedsSortvia#[command(flatten)]can set its own default (resolvingNonein code) without the flattened help advertising a wrong value. fgumi's own default (64) is documented only in fgumi's commandlong_about, which flatten does not propagate.Testing
cargo ci-fmt,cargo ci-lint(clippy pedantic), andcargo ci-testall pass (5738 tests). New tests cover parsing (unset / explicit), rejection of values< 2, and thebuild_sorterwiring (asserted against a freshly-constructed sorter's default rather than a hardcoded 64).Summary by CodeRabbit
--max-temp-files <usize>option to control when temporary spilled runs are consolidated.--max-temp-files, its trade-offs, and its constraints.