Repository navigation
feat(sort): variable-width template-coordinate sort key, zero-copy merge, and slimmer arena - #375
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
==========================================
- Coverage 91.01% 90.95% -0.07%
==========================================
Files 78 78
Lines 51197 51228 +31
==========================================
- Hits 46596 46593 -3
- Misses 4601 4635 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR generalizes template-coordinate BAM sorting to support lane-narrowed keys through a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…tead of per-record memcpy
…0B template per record)
Add fixed-width narrow alternatives to the full 56-byte template-coordinate key: TemplateKey24 (primary/secondary/name_hash_upper lanes) and TemplateKey32 (primary/secondary/opt/name_hash lanes), plus a TemplateKey40 alias and from_full constructors that project the full key onto each narrow layout.
…e selected variant through both phases Generalize the template buffer and radix passes over K: TemplateLaneKey and make the template-coordinate sort body generic over K, dispatching to the right monomorphization based on the selected variant. Variant selection (select_template_variant) derives the retained lanes from the first record's key or an explicit KeyTypesSpec. The wrapper peeks the first record, selects the variant, and hands the captured first record plus the rest of the iterator to sort_template_coordinate_impl<K>, which processes the first record in a pre-loop block and drains the remainder in the read loop. The decode-time verify (verify_dropped_lanes) runs on every record and hard-errors via dropped_lane_error if a dropped lane varies from the first record. Add CbKey32/TertKey32 repr(transparent) newtypes over TemplateKey32 so the four-arm (cb, tertiary) dispatch picks the correct optional-lane source at monomorphization with zero runtime cost. Add a key_types: KeyTypesSpec field (default Auto) plus builder setter to RawExternalSorter; replace the TemplateKey-typed merge_chunks_keyed wrapper with a direct merge_chunks_generic::<K> call. Size the template arena estimator to the chosen key width instead of the hardcoded pre-trim layout: data_bytes_per_record = TEMPLATE_HEADER_SIZE + EST_BAM_BYTES_PER_TEMPLATE_RECORD (8 + 250 B) and ref_bytes = size_of::<TemplateRecordRef<K>>() (40/48/56 B for TemplateKey24/32/40). The data split is now computed from the actual ref size rather than a fixed 86% fraction, and estimated_records gains a .max(1) floor. A new estimator-consistency test pins the ref sizes for all three key widths.
…anes Expose `--key-types <SPEC>` on `fgumi sort` (template-coordinate only). Parse it into `KeyTypesSpec` via `parse_key_types` and thread it into the `RawExternalSorter` builder. Non-template orders emit an info log and treat the flag as a no-op; the verify path is left unchanged. Re-export `KeyTypesSpec` from `fgumi_sort` alongside the other external sorter types.
Prove the correctness bar for --key-types: for each mode (bulk, single-cell, post-group, multi-library, full) a narrow-key template-coordinate sort produces a record stream byte-identical to the --key-types full baseline on the same input, and the Auto-detected variant matches too. Compares per-record BAM bodies in file order (not file bytes, which vary with BGZF block batching). Adds a spill-forced byte-identity case (tiny memory limit, multiple chunks) to exercise the Phase-2 k-way merge where the serialized narrow-key width must equal the Phase-1 width, an anti-vacuity sanity test asserting each mode's first record realizes the nonzero cb_hash/tertiary it claims, and a verify-after- narrow-sort test re-checking a narrow output at full width via core_cmp. Also lands the deferred decode-time hard-error integration tests: a later record that introduces CB / MI / a differing library ordinal absent from the first record makes a lite (--key-types none) sort return an error naming the --key-types token that re-includes the lane, rather than silently mis-sorting.
…y/full widths Add rstest-parameterized `narrow_key_spill_merge_byte_identical_all_widths` covering all five key-type modes (bulk/single-cell/post-group/multi-lib/full). Each case sorts the same input twice under a 32 KiB memory limit to force Phase-1 spills, once with `KeyTypesSpec::Full` as baseline and once with the mode's narrow spec, then asserts (a) both runs spilled >=2 chunks so the Phase-2 k-way merge at that key width is genuinely exercised, and (b) the resulting record byte-streams are identical. The existing `narrow_key_sort_byte_identical_when_spilling` test only covered the 24-byte (bulk/lite) path; this closes the gap for the 32-byte `CbKey32` (single-cell), 32-byte `TertKey32` (post-group, multi-lib), and 40-byte `TemplateKey40` (full) Phase-2 merge paths.
… is constant
Under KeyTypesSpec::Auto the template-coordinate sort previously kept the
tertiary library|mi lane whenever the first record's tertiary != 0. A
single-library BAM assigns its lone library ordinal 1 (ordinal 0 is reserved
for unknown/no-LB), so tertiary was nonzero even though the library lane is
constant across every record and therefore droppable.
Refine Auto to keep the tertiary lane only when it can actually vary: the
first record carries an MI tag (low-48 bits of tertiary nonzero) OR the header
realizes more than one distinct library ordinal. Detection counts distinct
ordinals realized by header read groups (not distinct LB strings), so a
single-LB header that also has LB-less RGs (ordinals {0,1}) still keeps the
lane. The header only informs provisioning; the decode-time verify still
guarantees a dropped lane was constant and hard-errors otherwise.
This narrows single-library inputs (e.g. 1000-Genomes WES/WGS: one library,
no MI) to the 24-byte lite key while preserving byte-identical output, as the
new byte-identity test confirms.
Merged
nh13
added a commit
that referenced
this pull request
Oct 14, 2026
…reaming spill writer
Every sort, merge and simulate now runs on the chain, so the v0.7.0 standalone
engine goes: external.rs (RawExternalSorter and its merge loops), the worker
pool, the pooled BAM and chunk writers, the streaming spill writer
(write_sorted_chunk), the zstd spill stream, the engine pipeline, and the merge
stall/trace/phase diagnostics. read_ahead.rs keeps only RawReadAheadReader,
memory_probe.rs only the RSS probe and mimalloc controls, and the unused
BatchedRawBamReader goes.
- fgumi_lib::sort::sort_bam(SortConfig) is the library entry: it builds the
chain `fgumi sort` runs. SortStats is Clone/Copy and the sort's summary hook
reads it without taking it, so a library caller gets it after the run.
- CoordinateChunkSorter::new(memory_limit, n_ref, sort_threads) and
TemplateChunkSorter::new(memory_limit, TemplateKeySeed, sort_threads) replace
the into_*_chunk_sorter builders.
- `fgumi sort --read-streams` takes fgumi_bam_io::ReadStreams.
- fgumi-sort's README describes the crate as engine components and points at
sort_bam and the CLI.
- bytesize and crossbeam-queue are no longer fgumi-sort dependencies.
- `fgumi sort`, sort_bam and `fgumi simulate` stage their output beside the
destination and publish it (with its BAI) only when the run succeeded and
wrote every record it read; a failed or short run leaves the destination as
it was. This fixes a pre-existing CLI behaviour: since the chain cutover a
failed `fgumi sort` left a truncated BAM at the destination, and the
engine's record-count disposition had no chain equivalent.
- The engine-only in-memory merge source (MemorySources,
SlotMergeSource::Memory), RecordBuffer::par_sort_into_chunks, ProbeableBuffer
and the dead TemplateRecordBuffer methods go; MergeDriver::from_slots takes
the residual chunks directly. The blanket dead_code allows on inline.rs and
reader.rs go with them.
- TemplateChunkSorter::push rejects a record shorter than the BAM core.
- fgumi_sort::resolve_temp_file_limit goes (no caller in fgumi or mako);
temp_file_limit_from_nofile(soft_nofile()) is the same number.
- Docs that described the retired engine as current are rewritten, including
the crate docs and description.
No behavioural test is deleted without a chain-level twin. The audit of the
retired engine's tests:
Every test in `crates/fgumi-sort/src/external.rs` `mod tests`, plus `crates/fgumi-sort/tests/integration/{test_read_ahead,test_output_compression}.rs`, with its fate. The other `external.rs` test modules (`from_slots_merge_tests`, `kind_dispatch_tests`, `bam_slot_tests`, `slot_ingest_tests`) test surviving code and move to `merge_driver.rs` in commit 4. Paths: `TC` = `tests/integration/test_sort_correctness.rs`, `MTF` = `tests/integration/test_sort_max_temp_files.rs`, `SIM` = `tests/integration/test_simulate_sort.rs`, `MP` = `tests/integration/test_merge_ported.rs`, `PIO` = `crates/fgumi-pipeline-io/src/sort/tests.rs`, `CMD` = `src/lib/commands/sort.rs` tests. "new" = written in D-4.
Summary (external.rs `mod tests`, 114 tests): 38 move with their code; 55 are deleted with a chain-level twin (13 new twin tests written in D-4, most of them rstest matrices, plus the staging tests); 21 are deleted as n/a (engine API or internals with no chain counterpart). Plus the 4 tests of `fgumi-sort/tests/integration/test_{read_ahead,output_compression}.rs`, deleted with twins.
## Move with surviving code (no twin needed)
| tests | code | destination |
| --- | --- | --- |
| test_library_lookup_empty_header, _single_rg, _multiple_libraries, _unknown_rg_returns_zero, test_distinct_header_ordinals | LibraryLookup | template_key.rs |
| test_find_rg_tag, test_find_rg_tag_after_other_tags | RG lookup (`find_string`) used by LibraryLookup | template_key.rs |
| test_cb_hasher | cb_hasher | template_key.rs |
| test_extract_template_key_cb_present_has_nonzero_hash, _cb_absent_has_zero_hash, _cell_tag_none_has_zero_hash, _different_cb_values_differ, _cb_hash_is_deterministic, _secondary_supplementary_uses_tc_tag, _secondary_supplementary_without_tc_falls_back, _secondary_supplementary_matches_primary_via_tc, _unmapped_with_cb | extract_template_key_inline | template_key.rs |
| verify_detects_cb_appearing, verify_detects_mi_change, verify_detects_library_change, verify_passes_when_dropped_lanes_constant, verify_error_message_names_field_and_token | verify_dropped_lanes, dropped_lane_error | template_key.rs |
| auto_lite_when_no_cb_no_tertiary, auto_cb_only_when_first_record_has_cb, auto_tertiary_only_when_first_record_has_mi_or_library, auto_full_when_both_present, auto_drops_constant_single_library_lane, auto_keeps_tertiary_when_header_library_varies, auto_keeps_tertiary_for_mi_even_with_constant_library, none_forces_lite, full_forces_all_lanes, explicit_spec_passthrough, empty_input_defaults_to_lite, ref_width_matches_the_ref_size | select_template_variant, TemplateKeyVariant | template_key.rs |
| test_read_exact_or_eof_clean_eof, _full_read, _short_reads_fill_buffer, _truncated_is_error | read_exact_or_eof | spill_block_reader.rs |
## Deleted, with a chain-level twin
| engine test | what it checks | twin |
| --- | --- | --- |
| phase_thread_split_is_byte_identical, residual_phase1_honors_sort_threads_with_single_base_thread, test_per_phase_threads_produce_identical_output | per-phase thread splits do not change output | CMD::test_execute_with_per_phase_threads_is_output_identical; tests/integration/test_sort_phase_caps.rs |
| test_raw_sorter_sort_rejects_temp_compression_zero_with_zstd | zstd level 0 rejected | CMD::test_temp_compression_zero_with_zstd_rejected |
| test_fd_budget_warning_names_the_width_and_the_lever, test_fd_budget_warning_reads_the_process_budget | descriptor-budget warning | CMD::test_fd_budget_warning (the CLI's own warning) |
| test_create_output_header_coordinate, _queryname, _template_coordinate | output @hd SO/GO/SS | TC::the_output_header_declares_the_order (new) |
| template_sort_auto_lite_roundtrips_record_count | auto key round-trips every record | TC::every_key_width_matches_the_full_key_when_dropped_lanes_are_constant (new, auto case) |
| template_sort_empty_input_writes_header_only_bam, test_merge_chunks_generic_with_only_empty_sources, test_merge_chunks_with_index_with_only_empty_sources | empty input → header-only output | TC::an_empty_input_writes_a_header_only_bam (new); PIO::test_sort_merge_empty_input_merges_to_empty |
| test_sort_with_consolidation_preserves_all_records, test_consolidation_while_a_run_is_open | consolidation keeps every record, sorted | MTF::consolidation_bounds_merge_sources_without_changing_the_output, MTF::max_temp_files_consolidation_is_record_identical (new port of the CMD test); consolidate_runs/tests.rs |
| test_sort_queryname_preserves_exact_tie_input_order, test_sort_queryname_output_identity_matches_stable_baseline | queryname ties keep input order; output = stable sort | PIO::ties_across_run_boundaries_keep_input_order, PIO::queryname_split_matches_reference_across_runs; bytecheck vs v0.7.0 |
| test_presorted_input_collapses_to_one_run, test_run_formation_output_is_byte_identical | presorted input spills one run, output unchanged | PIO::presorted_bam_coalesces_to_one_run_through_the_arena_split; spill_gather/tests.rs::many_presorted_chunks_collapse_to_a_single_run |
| test_unsorted_input_never_collapses_to_one_run, test_descent_opens_exactly_one_more_run | run boundaries open where order breaks | spill_gather/tests.rs::contiguous_chunks_coalesce_into_one_run, ::exact_boundary_tie_extends_the_run_for_content_keys; PIO::shuffled_bam_matches_the_oracle_through_the_arena_split |
| test_append_across_spill_codecs | run extension under both codecs | MTF::uncompressed_bgzf_spills_match_the_default_codec |
| test_appending_run_with_write_index, test_sort_coordinate_with_index_in_memory_writes_sidecar, test_sort_coordinate_with_index_spilled_preserves_records, test_write_index_oversized_record_builds_loadable_bai | `--write-index` loadable BAI, records unchanged | TC::write_index_writes_a_loadable_bai_and_the_same_records (new; in memory + spilled, oversized record); test_sort_write_index.rs (samtools) |
| test_oversized_record_survives_spill_and_pool_merge | a record wider than a BGZF block survives spill+merge | TC::a_record_wider_than_a_bgzf_block_survives_a_spill_and_merge (new) |
| test_sort_many_chunks_with_semaphore, test_sort_sub_arrays_match_single_thread, test_sort_sub_arrays_in_memory_only | many spills / threads give the single-thread output | tests/integration/test_sort_cutover_parity.rs::cutover_spill_and_nonspill_are_identical; PIO::ties_across_run_boundaries_keep_input_order (all thread counts); TC::test_sort_coordinate_consistent_across_threads (samtools) |
| test_sort_with_two_temp_dirs_matches_single_dir | two temp dirs = one | TC::two_temp_dirs_match_a_single_dir (new) |
| record_count_mismatch_removes_the_output, _never_unlinks_stdout, _removes_the_symlink_target, _leaves_a_non_regular_output_alone, _tolerates_a_missing_output, matching_record_counts_leave_the_output_alone | a sort that loses records leaves no output | the chain now stages its output (`StagedSortOutput`, D-4) and publishes only when the run succeeded and wrote every record it read: staged_output.rs::a_record_count_mismatch_publishes_nothing, ::a_complete_run_publishes_the_bam_and_its_index; tests/integration/test_sort_bam_api.rs::a_failed_sort_leaves_the_destination_as_it_was (CLI and API, with and without an index, new and existing destination); SIM::a_failed_simulate_leaves_an_existing_destination_as_it_was. stdout is streamed and never staged; a symlinked destination is followed and a non-regular one refused (`MergeOutputTarget` tests in output_target.rs). |
| test_sort_records_matches_sort_from_file | in-process stream sort = file sort | SIM::simulate_output_matches_the_reference_for_a_fixed_seed |
| test_sort_records_empty_stream_writes_header_only | empty stream → header-only | SIM::an_empty_generator_writes_a_header_only_output (new) |
| test_sort_records_producer_error_aborts | producer error fails, no output | SIM::aborted_simulate_leaves_no_output, SIM::generator_error_fails_the_pipeline |
| test_sort_records_error_on_first_record_aborts | error before any record fails | SIM::an_error_before_any_record_fails_the_pipeline (new) |
| test_merge_bams_coordinate_sort, _template_coordinate_sort, _queryname_sort, _single_input, _preserves_all_records, _many_inputs, _queryname_natural_sort | `merge_bams` | MP::test_merge_ported_* (one per engine test, same assertions, via `fgumi merge`) |
| narrow_key_sort_byte_identical_to_full, narrow_key_sort_byte_identical_when_spilling, narrow_key_spill_merge_byte_identical_all_widths, auto_drops_constant_library_lane_byte_identical, narrow_sort_output_passes_full_width_verify | narrow keys write the full key's output when dropped lanes are constant | TC::every_key_width_matches_the_full_key_when_dropped_lanes_are_constant (new; none/cb/mi/cb,mi/auto × memory × threads; byte identity to the full key implies full-width order); MTF::consolidation_preserves_every_template_coordinate_key_lane |
| lite_sort_hard_errors_when_cb_appears, _when_mi_appears, _when_library_differs | varying dropped lane fails with the re-include token | TC::a_dropped_lane_that_varies_fails_the_sort (new); PIO::violation_report_is_identical_across_template_paths, ::sort_buffer_chain_fails_the_pipeline_on_a_dropped_lane_violation |
| auto_single_library_multi_readgroup_sorts_without_false_violation, auto_single_library_unmapped_read_sorts_without_false_violation | #375: no false library violation | TC::a_single_library_input_sorts_under_auto_without_a_false_violation (new; in memory + spilled) |
| owned_template_sort_reports_a_violation_whose_read_name_overruns_its_body | malformed name in a violation is an error, not a panic | TC::a_violation_on_a_record_with_an_overrunning_read_name_is_an_error (new) |
## Deleted, n/a (engine API or internals)
| engine test | reason |
| --- | --- |
| test_raw_sorter_defaults, test_raw_sorter_builder_chain, test_raw_sorter_phase_threads_builders, test_raw_sorter_memory_limit, test_raw_sorter_temp_compression, test_raw_sorter_max_temp_files, test_raw_sorter_cell_tag_default_is_none, test_raw_sorter_cell_tag_builder, test_phase_thread_defaults | builder/getters of the deleted `RawExternalSorter`. The CLI's defaults are pinned by CMD::sort_options_default_matches_cli_defaults and the phase-thread tests in CMD. |
| block_lifecycle_gate_covers_every_report, merge_stall_gate_covers_every_report, silent_stall_gate_does_not_silence_the_lifecycle_gate | engine merge-stall/lifecycle diagnostics (merge_stalls.rs, merge_trace.rs, deleted). Chain diagnostics: tests/integration/test_sort_stats_diagnostics.rs. |
| test_phase2_guard_releases_sources_before_finalizing_output, test_phase2_guard_finish_output_resets_phase_on_error | engine `Phase2Guard` / worker-pool phase |
| test_sort_phase_timer_all_methods | engine `SortPhaseTimer`; chain phase timing is `SortPhaseTimingFinalizeHook` |
| test_max_chunk_records_keeps_ingest_positions_in_u32, test_should_spill_chunk | engine chunk-cap constants |
| test_split_off_aligns_with_par_chunks_mut | engine sub-array split |
| estimator_bytes_per_record_matches_ref_size | engine `EST_BAM_BYTES_PER_TEMPLATE_RECORD` estimator (only `into_template_chunk_sorter`/the engine used it) |
| mode_inputs_realize_claimed_lanes, cluster_barcodes_have_distinct_cb_hashes | validate the deleted narrow-key fixtures; the new twin's fixture is a single constant-lane input |
## crates/fgumi-sort/tests/integration
| test | fate |
| --- | --- |
| test_read_ahead.rs (2 tests: coordinate and template-coordinate sort through `RawExternalSorter` + pool read-ahead) | deleted; twins: TC chain sorts at t1/t4 (every test above), test_sort_native_reads.rs::native_reads_are_byte_identical |
| test_output_compression.rs (2 rstest tests: `--compression-level` reaches the output, `--temp-compression` never does, in memory and spilling) | deleted; twin TC::the_output_level_reaches_the_bam_and_the_temp_level_does_not (new; 3 orders × memory × threads) |
| test_verify.rs | kept; sorts with the local stable `local_reference_sort` (common.rs, with `local_reference_sort_is_stable`) instead of `RawExternalSorter` |
## Other tests deleted with engine code (outside external.rs)
| tests | reason |
| --- | --- |
| every test in the deleted `worker_pool.rs`, `bgzf_io.rs`, `pooled_bam_writer.rs`, `pooled_chunk_writer.rs`, `zspill_stream.rs`, `sync_spill_writer.rs`, `pipeline.rs`, `merge_stalls.rs`, `merge_trace.rs`, `merge_phases.rs` | test the deleted engine modules themselves. The four `test_read_length_prefix_*` tests moved to `spill_block_reader.rs` with `read_length_prefix` (commit 4). |
| `spill_block.rs::kernel_matches_streaming_oracle_read_back` | its oracle was the deleted `write_sorted_chunk`; `kernel_blocks_read_back_records` (kept, now read back through `SpillBlockDecompressor` instead of the deleted `GenericKeyedChunkReader`) checks the stronger property that the kernel's file reads back to the input records |
| `spill_block_reader.rs` round-trip tests | kept; their fixture writer moved from the deleted `PooledChunkWriter` to the production block kernel (`frame_keyed_record_into` + `SpillBlockCompressor`) |
| `read_ahead.rs` `PooledInputStream` / `RecordSource` tests (block straddle, borrowed-vs-owned, proptest) | the types were engine-only (pool input stream) and are deleted; `RawReadAheadReader`'s tests are kept |
| `memory_probe.rs` `fmt_bytes`, `log_snapshot`, `SpillProbe`, `MergeProbe` tests | the probes were engine-only and are deleted; the RSS and purge-delay tests are kept |
| `lib.rs` `test_header_declares_order_*` (7) | `header_declares_order` only fed the engine's "input already declares this order" warning; the chain never issued it (unchanged since the cutover) |
| `inline.rs::test_in_memory_chunk_take_key_replaces_with_default` | `InMemoryChunk::take_key`'s sole user was the engine's `ChunkSource::Memory` |
| `reader.rs::test_batched_reader`, `test_batched_reader_eof` | `BatchedRawBamReader` had no user |
| chunk_sorter.rs / template_arena.rs tests | kept; built with the new direct constructors |
| `inline.rs` `test_par_sort_into_chunks_{coordinate,template,handles_unmapped_sentinel,shares_arc_across_siblings}`, `par_sort_into_chunks_returns_the_pooled_arena_when_its_chunks_drop` | `par_sort_into_chunks` fed only the engine's with-spill sub-run path; the chain emits one globally sorted chunk (`RecordBuffer::par_sort`, tested). Deleted with the method (review round 1, G25) |
| `merge_driver/tests.rs::from_slots_merges_slots_with_memory_chunks` | `SlotMergeSource::Memory`/`MemorySources` were engine-only; `from_slots` now takes the residual `InMemoryChunk`s directly, and the remaining from_slots tests cover it (review round 1, G25) |
BREAKING CHANGE: fgumi-sort no longer ships a standalone sorter.
RawExternalSorter and its builders, the worker pool, PooledBamWriter,
PooledChunkWriter, the streaming spill writer, the engine pipeline and its
merge diagnostics, BatchedRawBamReader, MemorySources,
SlotMergeSource::Memory, RecordBuffer::par_sort_into_chunks,
the into_*_chunk_sorter builders, the fgumi_sort::ReadStreams re-export and
fgumi_sort::resolve_temp_file_limit are removed. Use
fgumi_lib::sort::sort_bam (or `fgumi sort`) to sort, the
CoordinateChunkSorter::new / TemplateChunkSorter::new constructors, and
temp_file_limit_from_nofile(soft_nofile()) for the descriptor-derived
limit.
nh13
added a commit
that referenced
this pull request
Oct 14, 2026
…reaming spill writer
Every sort, merge and simulate now runs on the chain, so the v0.7.0 standalone
engine goes: external.rs (RawExternalSorter and its merge loops), the worker
pool, the pooled BAM and chunk writers, the streaming spill writer
(write_sorted_chunk), the zstd spill stream, the engine pipeline, and the merge
stall/trace/phase diagnostics. read_ahead.rs keeps only RawReadAheadReader,
memory_probe.rs only the RSS probe and mimalloc controls, and the unused
BatchedRawBamReader goes.
- fgumi_lib::sort::sort_bam(SortConfig) is the library entry: it builds the
chain `fgumi sort` runs. SortStats is Clone/Copy and the sort's summary hook
reads it without taking it, so a library caller gets it after the run.
- CoordinateChunkSorter::new(memory_limit, n_ref, sort_threads) and
TemplateChunkSorter::new(memory_limit, TemplateKeySeed, sort_threads) replace
the into_*_chunk_sorter builders.
- `fgumi sort --read-streams` takes fgumi_bam_io::ReadStreams.
- fgumi-sort's README describes the crate as engine components and points at
sort_bam and the CLI.
- bytesize and crossbeam-queue are no longer fgumi-sort dependencies.
- `fgumi sort`, sort_bam and `fgumi simulate` stage their output beside the
destination and publish it (with its BAI) only when the run succeeded and
wrote every record it read; a failed or short run leaves the destination as
it was. This fixes a pre-existing CLI behaviour: since the chain cutover a
failed `fgumi sort` left a truncated BAM at the destination, and the
engine's record-count disposition had no chain equivalent.
- The engine-only in-memory merge source (MemorySources,
SlotMergeSource::Memory), RecordBuffer::par_sort_into_chunks, ProbeableBuffer
and the dead TemplateRecordBuffer methods go; MergeDriver::from_slots takes
the residual chunks directly. The blanket dead_code allows on inline.rs and
reader.rs go with them.
- TemplateChunkSorter::push rejects a record shorter than the BAM core.
- fgumi_sort::resolve_temp_file_limit goes (no caller in fgumi or mako);
temp_file_limit_from_nofile(soft_nofile()) is the same number.
- Docs that described the retired engine as current are rewritten, including
the crate docs and description.
No behavioural test is deleted without a chain-level twin. The audit of the
retired engine's tests:
Every test in `crates/fgumi-sort/src/external.rs` `mod tests`, plus `crates/fgumi-sort/tests/integration/{test_read_ahead,test_output_compression}.rs`, with its fate. The other `external.rs` test modules (`from_slots_merge_tests`, `kind_dispatch_tests`, `bam_slot_tests`, `slot_ingest_tests`) test surviving code and move to `merge_driver.rs` in commit 4. Paths: `TC` = `tests/integration/test_sort_correctness.rs`, `MTF` = `tests/integration/test_sort_max_temp_files.rs`, `SIM` = `tests/integration/test_simulate_sort.rs`, `MP` = `tests/integration/test_merge_ported.rs`, `PIO` = `crates/fgumi-pipeline-io/src/sort/tests.rs`, `CMD` = `src/lib/commands/sort.rs` tests. "new" = written in D-4.
Summary (external.rs `mod tests`, 114 tests): 38 move with their code; 55 are deleted with a chain-level twin (13 new twin tests written in D-4, most of them rstest matrices, plus the staging tests); 21 are deleted as n/a (engine API or internals with no chain counterpart). Plus the 4 tests of `fgumi-sort/tests/integration/test_{read_ahead,output_compression}.rs`, deleted with twins.
## Move with surviving code (no twin needed)
| tests | code | destination |
| --- | --- | --- |
| test_library_lookup_empty_header, _single_rg, _multiple_libraries, _unknown_rg_returns_zero, test_distinct_header_ordinals | LibraryLookup | template_key.rs |
| test_find_rg_tag, test_find_rg_tag_after_other_tags | RG lookup (`find_string`) used by LibraryLookup | template_key.rs |
| test_cb_hasher | cb_hasher | template_key.rs |
| test_extract_template_key_cb_present_has_nonzero_hash, _cb_absent_has_zero_hash, _cell_tag_none_has_zero_hash, _different_cb_values_differ, _cb_hash_is_deterministic, _secondary_supplementary_uses_tc_tag, _secondary_supplementary_without_tc_falls_back, _secondary_supplementary_matches_primary_via_tc, _unmapped_with_cb | extract_template_key_inline | template_key.rs |
| verify_detects_cb_appearing, verify_detects_mi_change, verify_detects_library_change, verify_passes_when_dropped_lanes_constant, verify_error_message_names_field_and_token | verify_dropped_lanes, dropped_lane_error | template_key.rs |
| auto_lite_when_no_cb_no_tertiary, auto_cb_only_when_first_record_has_cb, auto_tertiary_only_when_first_record_has_mi_or_library, auto_full_when_both_present, auto_drops_constant_single_library_lane, auto_keeps_tertiary_when_header_library_varies, auto_keeps_tertiary_for_mi_even_with_constant_library, none_forces_lite, full_forces_all_lanes, explicit_spec_passthrough, empty_input_defaults_to_lite, ref_width_matches_the_ref_size | select_template_variant, TemplateKeyVariant | template_key.rs |
| test_read_exact_or_eof_clean_eof, _full_read, _short_reads_fill_buffer, _truncated_is_error | read_exact_or_eof | spill_block_reader.rs |
## Deleted, with a chain-level twin
| engine test | what it checks | twin |
| --- | --- | --- |
| phase_thread_split_is_byte_identical, residual_phase1_honors_sort_threads_with_single_base_thread, test_per_phase_threads_produce_identical_output | per-phase thread splits do not change output | CMD::test_execute_with_per_phase_threads_is_output_identical; tests/integration/test_sort_phase_caps.rs |
| test_raw_sorter_sort_rejects_temp_compression_zero_with_zstd | zstd level 0 rejected | CMD::test_temp_compression_zero_with_zstd_rejected |
| test_fd_budget_warning_names_the_width_and_the_lever, test_fd_budget_warning_reads_the_process_budget | descriptor-budget warning | CMD::test_fd_budget_warning (the CLI's own warning) |
| test_create_output_header_coordinate, _queryname, _template_coordinate | output @hd SO/GO/SS | TC::the_output_header_declares_the_order (new) |
| template_sort_auto_lite_roundtrips_record_count | auto key round-trips every record | TC::every_key_width_matches_the_full_key_when_dropped_lanes_are_constant (new, auto case) |
| template_sort_empty_input_writes_header_only_bam, test_merge_chunks_generic_with_only_empty_sources, test_merge_chunks_with_index_with_only_empty_sources | empty input → header-only output | TC::an_empty_input_writes_a_header_only_bam (new); PIO::test_sort_merge_empty_input_merges_to_empty |
| test_sort_with_consolidation_preserves_all_records, test_consolidation_while_a_run_is_open | consolidation keeps every record, sorted | MTF::consolidation_bounds_merge_sources_without_changing_the_output, MTF::max_temp_files_consolidation_is_record_identical (new port of the CMD test); consolidate_runs/tests.rs |
| test_sort_queryname_preserves_exact_tie_input_order, test_sort_queryname_output_identity_matches_stable_baseline | queryname ties keep input order; output = stable sort | PIO::ties_across_run_boundaries_keep_input_order, PIO::queryname_split_matches_reference_across_runs; bytecheck vs v0.7.0 |
| test_presorted_input_collapses_to_one_run, test_run_formation_output_is_byte_identical | presorted input spills one run, output unchanged | PIO::presorted_bam_coalesces_to_one_run_through_the_arena_split; spill_gather/tests.rs::many_presorted_chunks_collapse_to_a_single_run |
| test_unsorted_input_never_collapses_to_one_run, test_descent_opens_exactly_one_more_run | run boundaries open where order breaks | spill_gather/tests.rs::contiguous_chunks_coalesce_into_one_run, ::exact_boundary_tie_extends_the_run_for_content_keys; PIO::shuffled_bam_matches_the_oracle_through_the_arena_split |
| test_append_across_spill_codecs | run extension under both codecs | MTF::uncompressed_bgzf_spills_match_the_default_codec |
| test_appending_run_with_write_index, test_sort_coordinate_with_index_in_memory_writes_sidecar, test_sort_coordinate_with_index_spilled_preserves_records, test_write_index_oversized_record_builds_loadable_bai | `--write-index` loadable BAI, records unchanged | TC::write_index_writes_a_loadable_bai_and_the_same_records (new; in memory + spilled, oversized record); test_sort_write_index.rs (samtools) |
| test_oversized_record_survives_spill_and_pool_merge | a record wider than a BGZF block survives spill+merge | TC::a_record_wider_than_a_bgzf_block_survives_a_spill_and_merge (new) |
| test_sort_many_chunks_with_semaphore, test_sort_sub_arrays_match_single_thread, test_sort_sub_arrays_in_memory_only | many spills / threads give the single-thread output | tests/integration/test_sort_cutover_parity.rs::cutover_spill_and_nonspill_are_identical; PIO::ties_across_run_boundaries_keep_input_order (all thread counts); TC::test_sort_coordinate_consistent_across_threads (samtools) |
| test_sort_with_two_temp_dirs_matches_single_dir | two temp dirs = one | TC::two_temp_dirs_match_a_single_dir (new) |
| record_count_mismatch_removes_the_output, _never_unlinks_stdout, _removes_the_symlink_target, _leaves_a_non_regular_output_alone, _tolerates_a_missing_output, matching_record_counts_leave_the_output_alone | a sort that loses records leaves no output | the chain now stages its output (`StagedSortOutput`, D-4) and publishes only when the run succeeded and wrote every record it read: staged_output.rs::a_record_count_mismatch_publishes_nothing, ::a_complete_run_publishes_the_bam_and_its_index; tests/integration/test_sort_bam_api.rs::a_failed_sort_leaves_the_destination_as_it_was (CLI and API, with and without an index, new and existing destination); SIM::a_failed_simulate_leaves_an_existing_destination_as_it_was. stdout is streamed and never staged; a symlinked destination is followed and a non-regular one refused (`MergeOutputTarget` tests in output_target.rs). |
| test_sort_records_matches_sort_from_file | in-process stream sort = file sort | SIM::simulate_output_matches_the_reference_for_a_fixed_seed |
| test_sort_records_empty_stream_writes_header_only | empty stream → header-only | SIM::an_empty_generator_writes_a_header_only_output (new) |
| test_sort_records_producer_error_aborts | producer error fails, no output | SIM::aborted_simulate_leaves_no_output, SIM::generator_error_fails_the_pipeline |
| test_sort_records_error_on_first_record_aborts | error before any record fails | SIM::an_error_before_any_record_fails_the_pipeline (new) |
| test_merge_bams_coordinate_sort, _template_coordinate_sort, _queryname_sort, _single_input, _preserves_all_records, _many_inputs, _queryname_natural_sort | `merge_bams` | MP::test_merge_ported_* (one per engine test, same assertions, via `fgumi merge`) |
| narrow_key_sort_byte_identical_to_full, narrow_key_sort_byte_identical_when_spilling, narrow_key_spill_merge_byte_identical_all_widths, auto_drops_constant_library_lane_byte_identical, narrow_sort_output_passes_full_width_verify | narrow keys write the full key's output when dropped lanes are constant | TC::every_key_width_matches_the_full_key_when_dropped_lanes_are_constant (new; none/cb/mi/cb,mi/auto × memory × threads; byte identity to the full key implies full-width order); MTF::consolidation_preserves_every_template_coordinate_key_lane |
| lite_sort_hard_errors_when_cb_appears, _when_mi_appears, _when_library_differs | varying dropped lane fails with the re-include token | TC::a_dropped_lane_that_varies_fails_the_sort (new); PIO::violation_report_is_identical_across_template_paths, ::sort_buffer_chain_fails_the_pipeline_on_a_dropped_lane_violation |
| auto_single_library_multi_readgroup_sorts_without_false_violation, auto_single_library_unmapped_read_sorts_without_false_violation | #375: no false library violation | TC::a_single_library_input_sorts_under_auto_without_a_false_violation (new; in memory + spilled) |
| owned_template_sort_reports_a_violation_whose_read_name_overruns_its_body | malformed name in a violation is an error, not a panic | TC::a_violation_on_a_record_with_an_overrunning_read_name_is_an_error (new) |
## Deleted, n/a (engine API or internals)
| engine test | reason |
| --- | --- |
| test_raw_sorter_defaults, test_raw_sorter_builder_chain, test_raw_sorter_phase_threads_builders, test_raw_sorter_memory_limit, test_raw_sorter_temp_compression, test_raw_sorter_max_temp_files, test_raw_sorter_cell_tag_default_is_none, test_raw_sorter_cell_tag_builder, test_phase_thread_defaults | builder/getters of the deleted `RawExternalSorter`. The CLI's defaults are pinned by CMD::sort_options_default_matches_cli_defaults and the phase-thread tests in CMD. |
| block_lifecycle_gate_covers_every_report, merge_stall_gate_covers_every_report, silent_stall_gate_does_not_silence_the_lifecycle_gate | engine merge-stall/lifecycle diagnostics (merge_stalls.rs, merge_trace.rs, deleted). Chain diagnostics: tests/integration/test_sort_stats_diagnostics.rs. |
| test_phase2_guard_releases_sources_before_finalizing_output, test_phase2_guard_finish_output_resets_phase_on_error | engine `Phase2Guard` / worker-pool phase |
| test_sort_phase_timer_all_methods | engine `SortPhaseTimer`; chain phase timing is `SortPhaseTimingFinalizeHook` |
| test_max_chunk_records_keeps_ingest_positions_in_u32, test_should_spill_chunk | engine chunk-cap constants |
| test_split_off_aligns_with_par_chunks_mut | engine sub-array split |
| estimator_bytes_per_record_matches_ref_size | engine `EST_BAM_BYTES_PER_TEMPLATE_RECORD` estimator (only `into_template_chunk_sorter`/the engine used it) |
| mode_inputs_realize_claimed_lanes, cluster_barcodes_have_distinct_cb_hashes | validate the deleted narrow-key fixtures; the new twin's fixture is a single constant-lane input |
## crates/fgumi-sort/tests/integration
| test | fate |
| --- | --- |
| test_read_ahead.rs (2 tests: coordinate and template-coordinate sort through `RawExternalSorter` + pool read-ahead) | deleted; twins: TC chain sorts at t1/t4 (every test above), test_sort_native_reads.rs::native_reads_are_byte_identical |
| test_output_compression.rs (2 rstest tests: `--compression-level` reaches the output, `--temp-compression` never does, in memory and spilling) | deleted; twin TC::the_output_level_reaches_the_bam_and_the_temp_level_does_not (new; 3 orders × memory × threads) |
| test_verify.rs | kept; sorts with the local stable `local_reference_sort` (common.rs, with `local_reference_sort_is_stable`) instead of `RawExternalSorter` |
## Other tests deleted with engine code (outside external.rs)
| tests | reason |
| --- | --- |
| every test in the deleted `worker_pool.rs`, `bgzf_io.rs`, `pooled_bam_writer.rs`, `pooled_chunk_writer.rs`, `zspill_stream.rs`, `sync_spill_writer.rs`, `pipeline.rs`, `merge_stalls.rs`, `merge_trace.rs`, `merge_phases.rs` | test the deleted engine modules themselves. The four `test_read_length_prefix_*` tests moved to `spill_block_reader.rs` with `read_length_prefix` (commit 4). |
| `spill_block.rs::kernel_matches_streaming_oracle_read_back` | its oracle was the deleted `write_sorted_chunk`; `kernel_blocks_read_back_records` (kept, now read back through `SpillBlockDecompressor` instead of the deleted `GenericKeyedChunkReader`) checks the stronger property that the kernel's file reads back to the input records |
| `spill_block_reader.rs` round-trip tests | kept; their fixture writer moved from the deleted `PooledChunkWriter` to the production block kernel (`frame_keyed_record_into` + `SpillBlockCompressor`) |
| `read_ahead.rs` `PooledInputStream` / `RecordSource` tests (block straddle, borrowed-vs-owned, proptest) | the types were engine-only (pool input stream) and are deleted; `RawReadAheadReader`'s tests are kept |
| `memory_probe.rs` `fmt_bytes`, `log_snapshot`, `SpillProbe`, `MergeProbe` tests | the probes were engine-only and are deleted; the RSS and purge-delay tests are kept |
| `lib.rs` `test_header_declares_order_*` (7) | `header_declares_order` only fed the engine's "input already declares this order" warning; the chain never issued it (unchanged since the cutover) |
| `inline.rs::test_in_memory_chunk_take_key_replaces_with_default` | `InMemoryChunk::take_key`'s sole user was the engine's `ChunkSource::Memory` |
| `reader.rs::test_batched_reader`, `test_batched_reader_eof` | `BatchedRawBamReader` had no user |
| chunk_sorter.rs / template_arena.rs tests | kept; built with the new direct constructors |
| `inline.rs` `test_par_sort_into_chunks_{coordinate,template,handles_unmapped_sentinel,shares_arc_across_siblings}`, `par_sort_into_chunks_returns_the_pooled_arena_when_its_chunks_drop` | `par_sort_into_chunks` fed only the engine's with-spill sub-run path; the chain emits one globally sorted chunk (`RecordBuffer::par_sort`, tested). Deleted with the method (review round 1, G25) |
| `merge_driver/tests.rs::from_slots_merges_slots_with_memory_chunks` | `SlotMergeSource::Memory`/`MemorySources` were engine-only; `from_slots` now takes the residual `InMemoryChunk`s directly, and the remaining from_slots tests cover it (review round 1, G25) |
BREAKING CHANGE: fgumi-sort no longer ships a standalone sorter.
RawExternalSorter and its builders, the worker pool, PooledBamWriter,
PooledChunkWriter, the streaming spill writer, the engine pipeline and its
merge diagnostics, BatchedRawBamReader, MemorySources,
SlotMergeSource::Memory, RecordBuffer::par_sort_into_chunks,
the into_*_chunk_sorter builders, the fgumi_sort::ReadStreams re-export and
fgumi_sort::resolve_temp_file_limit are removed. Use
fgumi_lib::sort::sort_bam (or `fgumi sort`) to sort, the
CoordinateChunkSorter::new / TemplateChunkSorter::new constructors, and
temp_file_limit_from_nofile(soft_nofile()) for the descriptor-derived
limit.
This branch was previously deployed
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
1 participant
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.
Three related, byte-identical improvements to the external merge-sort engine, landed as one stack. The sorted record stream is identical to current
mainfor every sort order (verified viasamtools view | md5— not raw.bambytes, since BGZF block batching is run-to-run nondeterministic).What's in it (suggested reading order)
d9c87b7,13d8f59) — the merge loop borrows in-memory record bytes from the arena instead ofmemcpy-ing each winning record into a per-source scratch buffer. Pure copy-elision; output unchanged.55ec938) — the per-record inline header carried a write-only copy of the sort key that is never read at merge time (the key is accessed via theRecordRef). Removing it shrinks the template inline header from 48 B to 8 B (coordinate 16 B to 8 B).--key-types(6ce9dca…dcc17f9) — three concrete key structsTemplateKey24/32/40(by u64-lane count) that drop provably-constant key lanes (cell barcode, and library/MI which share one lane), so the in-memory buffer holds more records. A single key type threads Phase 1 and Phase 2 so spill and merge widths agree by construction. The variant is auto-selected from the first record plus the header and is overridable via a new--key-types full|none|cb,library,miflag. A decode-time verify is the guarantor: it hard-errors if a dropped lane ever varies, so a narrow key can never silently mis-sort. Library is treated as per-record verify-gated, not header-trusted.Performance
Graviton4 (
c8g.8xlarge), gp3 500 MB/s, template-coordinate,-@8 -m 16 GiB, warm cache. Baseline = currentmain; "this PR" = the full stack.The win is concentrated where narrowing lets the dataset fit the memory budget (the panel goes to 0 spills). For inputs several times the budget (WES/WGS) it is neutral: those are spill-I/O-bound on the compressed record bytes, which key width does not touch, and peak RSS is governed by
--max-memory, not key width. Improvements 1 and 2 were neutral on wall in this run; they are included for the memory reduction and as the clean, byte-identical refactors the key work builds on. No workload regressed.Behavior note
Under default auto-detection a single-library input now narrows to the lite key. If such an input contains a read whose
@RGis absent from the header (so it realizes a different library ordinal than the first record), the sort now hard-errors with an actionable message (re-run with --key-types library) rather than silently keeping the wider key. This is safe-by-failure and does not fire for normal aligned BAMs, where every read carries a header read group.Testing
Full workspace test suite,
cargo ci-fmt, andcargo ci-lintall green. New unit, property, and integration coverage for each key type, the radix sort, variant selection, the decode-time verify, the CLI parser, and a byte-identity matrix across bulk / single-cell / post-group / multi-library / full modes, both in-memory and spill-forced at every key width. No newunsafe.New public surface
fgumi_sort::KeyTypesSpec(re-exported for the CLI). The key structs, newtypes, selection, and verify helpers remain crate-private.