M sstats big/work/20260513 arrow chunking followups#17
Merged
tonywu1999 merged 19 commits intoJul 10, 2026
Conversation
|
Warning Review limit reached
Next review available in: 38 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR switches Spectronaut conversion to Arrow record-batch streaming, rewrites chunk cleaning with ChangesSpectronaut Arrow Streaming & Data.Table Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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: 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 `@R/clean_spectronaut.R`:
- Around line 155-167: The current filter block assumes columns Excluded,
Identified, EGQvalue, PGQvalue, and FFrgLossType always exist and will error if
any are missing; update the logic in clean_spectronaut.R to guard each filter by
checking column presence (e.g., use "if ('Excluded' %in% names(input))" before
the Excluded filter, similarly for Identified, EGQvalue and PGQvalue before
applying qvalue-based NA assignment, and check for FFrgLossType before
subsetting), so each conditional only runs when its target column exists and the
behavior remains unchanged otherwise.
- Around line 25-58: The computed needed_cols is never passed to Arrow, so the
CSV reader still parses all columns; update the CsvConvertOptions usage to
include the projection by calling
arrow::CsvConvertOptions$create(include_columns = needed_cols) (or set the
include_columns field on convert_opts after creation) so that convert_opts
includes needed_cols before calling arrow::open_dataset/Scanner$create;
reference the symbols needed_cols and convert_opts (and the call
arrow::CsvConvertOptions$create) and ensure this happens prior to creating
ds/reader.
In `@tests/testthat/test-converters.R`:
- Around line 106-113: The test's expect_error calls are too broad—change them
to assert the error message mentions "block_size" so they only pass when
block_size validation fails; update each expect_error(reduceBigSpectronaut(...),
...) in tests/testthat/test-converters.R to include a second argument (string or
regex) that matches "block_size" (e.g., "block_size" or "block_size.*invalid")
for the negative values and vector cases, and similarly for the
suppressed-warning call so all invalid-block_size cases are checked by message
content rather than any error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ebfd981-78cf-43a3-b93a-fd55ccfaeccc
📒 Files selected for processing (7)
DESCRIPTIONNAMESPACER/clean_spectronaut.RR/converters.Rman/bigSpectronauttoMSstatsFormat.Rdman/dot-prefixedPath.Rdtests/testthat/test-converters.R
tonywu1999
reviewed
May 19, 2026
Rudhik1904
added a commit
that referenced
this pull request
Jul 2, 2026
…hten tests, document
R/clean_spectronaut.R: needed_cols was computed but never applied, so every
batch still materialized all ~35 columns. Wire it in via Scanner projection
(intersect(needed_cols, names(ds)) -> Scanner$create(projection=)). Note:
CsvConvertOptions$include_columns (the obvious fix) collides with the
open_dataset schema layer and errors ("Multiple matches for FieldRef.Name"),
so Scanner projection is the mechanism that works with the dataset API.
intersect() keeps only present columns so partial exports still read.
R/clean_spectronaut.R: cleanSpectronautChunk carefully tolerated missing
columns at subset/rename time, then assumed Excluded/Identified/EGQvalue/
PGQvalue/FFrgLossType existed in the filters -> cryptic data.table
"object not found". Added require_filter_cols() that fails fast with a clear
message naming the original Spectronaut column, scoped to when each filter
actually runs (chose fail-fast over silent skip: skipping a filter changes
which rows/intensities survive downstream).
tests/testthat/test-converters.R: added a test that stubs cleanSpectronautChunk
and asserts junk columns are projected out before the cleaner (guards the
projection; verified it fails if the projection is removed); added tests for
the fail-fast filter guards; tightened block_size expect_error() calls with
regexp = "block_size" so they only pass on the actual validation error.
R/converters.R, man/: expanded @param block_size with tuning guidance (memory
scales ~linearly, diminishing speed returns, double-and-watch strategy) and
added an @details section listing required Spectronaut columns split into
always-required / optional / required-per-filter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/testthat/test-converters.R (1)
237-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
expect_no_error()overexpect_error(x, NA).Functionally fine, but testthat's docs now recommend
expect_no_error()/expect_no_condition()in place ofexpect_error(object, NA)for asserting absence of an error: "If NA, asserts that there should be no errors, but we now recommend using expect_no_error() and friends instead."♻️ Suggested modernization
- expect_error( + expect_no_error( MSstatsBig:::cleanSpectronautChunk(make_spectronaut_input(n = 1L), - output_file, pos = 1L, - filter_by_qvalue = FALSE), - NA) + output_file, pos = 1L, + filter_by_qvalue = FALSE))🤖 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 `@tests/testthat/test-converters.R` around lines 237 - 241, Replace the `expect_error(..., NA)` assertion in the `cleanSpectronautChunk` test with the modern `expect_no_error()` helper. Update the test in the `test-converters` suite to keep the same inputs (`make_spectronaut_input`, `output_file`, `filter_by_qvalue`) while asserting that `MSstatsBig:::cleanSpectronautChunk` completes without error.
🤖 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.
Nitpick comments:
In `@tests/testthat/test-converters.R`:
- Around line 237-241: Replace the `expect_error(..., NA)` assertion in the
`cleanSpectronautChunk` test with the modern `expect_no_error()` helper. Update
the test in the `test-converters` suite to keep the same inputs
(`make_spectronaut_input`, `output_file`, `filter_by_qvalue`) while asserting
that `MSstatsBig:::cleanSpectronautChunk` completes without error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25f345c6-28cc-4482-9681-8556664282c4
📒 Files selected for processing (4)
R/clean_spectronaut.RR/converters.Rman/bigSpectronauttoMSstatsFormat.Rdtests/testthat/test-converters.R
✅ Files skipped from review due to trivial changes (1)
- man/bigSpectronauttoMSstatsFormat.Rd
🚧 Files skipped from review as they are similar to previous changes (2)
- R/converters.R
- R/clean_spectronaut.R
R/clean_spectronaut.R:9-12: added block_size parameter (default 16L * 1024L * 1024L) with coerce + validation. R/clean_spectronaut.R:44: CsvReadOptions$create now uses the parameter. R/converters.R:120-125: new @param block_size roxygen with the straddling-object workaround note. R/converters.R:148-156: bigSpectronauttoMSstatsFormat gains block_size, plumbed to reduceBigSpectronaut. tests/testthat/test-converters.R:97-163: validation tests (rejects negative/zero/NA/vector/string) + plumbing tests (default forwards 16 MiB, override forwards user's value). man/bigSpectronauttoMSstatsFormat.Rd: regenerated from roxygen.
…setnames so the package is data.table-aware (cedta()). R/clean_spectronaut.R:103-187: rewrote cleanSpectronautChunk in data.table: setDT(input) at entry; subsequent operations modify in place via :=. Two-step rename (setnames for standardize, then setnames with skip_absent = TRUE to map standardized→MSstats) matches the MSstatsConvert family pattern. Conditional NA assignment uses mask form dt[cond, Intensity := NA_real_]. Q-value filters preserve dplyr::if_else NA semantics via explicit is.na(EGQvalue) | EGQvalue >= cutoff. Dropped the leftover dplyr::collect(head(dplyr::select(...))) pattern — was a no-op residue from a prior refactor. Function shrank from ~88 lines to ~64. DESCRIPTION:20: added data.table to Imports. NAMESPACE: regenerated, now imports :=, .SD, setDT, setnames from data.table. tests/testthat/test-converters.R:97-211: 5 new tests — schema smoke test, filter_by_excluded, filter_by_identified, filter_by_qvalue (covering the NA-q-value case), and FFrgLossType row drop.
…hten tests, document
R/clean_spectronaut.R: needed_cols was computed but never applied, so every
batch still materialized all ~35 columns. Wire it in via Scanner projection
(intersect(needed_cols, names(ds)) -> Scanner$create(projection=)). Note:
CsvConvertOptions$include_columns (the obvious fix) collides with the
open_dataset schema layer and errors ("Multiple matches for FieldRef.Name"),
so Scanner projection is the mechanism that works with the dataset API.
intersect() keeps only present columns so partial exports still read.
R/clean_spectronaut.R: cleanSpectronautChunk carefully tolerated missing
columns at subset/rename time, then assumed Excluded/Identified/EGQvalue/
PGQvalue/FFrgLossType existed in the filters -> cryptic data.table
"object not found". Added require_filter_cols() that fails fast with a clear
message naming the original Spectronaut column, scoped to when each filter
actually runs (chose fail-fast over silent skip: skipping a filter changes
which rows/intensities survive downstream).
tests/testthat/test-converters.R: added a test that stubs cleanSpectronautChunk
and asserts junk columns are projected out before the cleaner (guards the
projection; verified it fails if the projection is removed); added tests for
the fail-fast filter guards; tightened block_size expect_error() calls with
regexp = "block_size" so they only pass on the actual validation error.
R/converters.R, man/: expanded @param block_size with tuning guidance (memory
scales ~linearly, diminishing speed returns, double-and-watch strategy) and
added an @details section listing required Spectronaut columns split into
always-required / optional / required-per-filter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tonywu1999
force-pushed
the
MSstatsBig/work/20260513_arrow_chunking_followups
branch
from
55132da to
74863fc
Compare
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
2 participants
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.
Motivation and Context
reduceBigSpectronaut()previously streamed Spectronaut CSV exports throughreadr::read_delim_chunked, which holds a string-interning pool that grows across chunks and pushed peak memory well above one batch's working set. This branch ports the reader to Arrow (which releases per-batch state) and follows up with the three remaining items fromTODO-arrow_chunking_followups.md:block_sizewas Arrow's 256 KiB default — too small for wide Spectronaut rows, causingInvalid: straddling object straddles two block boundarieserrors and excessive parser overhead.cleanSpectronautChunk()ran every batch through ~13 sequentialdplyrverbs on adata.frame, producing repeated transient column allocations and fragmenting R's allocator. The rest of theMSstatsConvertfamily isdata.table-native.Solution: replace
readrwith Arrow'sCsvReadOptions/Scanner/ToRecordBatchReader, raise the defaultblock_sizeto 16 MiB and expose it as a user parameter, document the straddling-object workaround at the parameter, and rewritecleanSpectronautChunk()in puredata.table.Changes
readr::read_delim_chunkedinreduceBigSpectronaut(). Usesarrow::open_dataset()+Scanner+ToRecordBatchReaderto stream record batches; preserves the existing comma/tab/semicolon delimiter switch viaCsvParseOptions$delimiter. Per-batch progress logging every 1,000 batches.block_sizeparameter on bothreduceBigSpectronaut()and the user-facingbigSpectronauttoMSstatsFormat(). Default16L * 1024L * 1024L(16 MiB) replaces Arrow's 256 KiB default. Coerced to integer and validated (length 1, non-NA, positive).@param block_sizedocuments the exact error string (Invalid: straddling object straddles two block boundaries) and the recommended override (64L * 1024L * 1024L) so users hitting the straddling error on pathological rows have a self-service fix.cleanSpectronautChunk()rewritten indata.table.setDT(input)at entry; column selection viadt[, cols, with = FALSE]; two-step rename viasetnames(..., skip_absent = TRUE)matching theMSstatsConvertfamily convention; in-place column updates via:=; conditional NA assignment via mask formdt[cond, Intensity := NA_real_]. Function shrank from ~88 lines to ~64.is.na(EGQvalue) | EGQvalue >= cutoffso rows with missing q-values still getIntensity = NA, matching the previousdplyr::if_elsebehavior (a naivedata.tabletranslation would silently change this).dplyr::collect(head(dplyr::select(...)))pattern at the old lines 140/144 was a no-op residue from an earlier Arrow-Table refactor and is gone.data.tableadded toImportsinDESCRIPTIONand imported via@importFrom data.table := .SD setDT setnamesso the package iscedta()-aware. RegeneratedNAMESPACEandman/bigSpectronauttoMSstatsFormat.Rdviadevtools::document().Testing
All tests run via
devtools::test(): 51 PASS, 0 FAIL, 0 WARN, 0 SKIP.New tests added in
tests/testthat/test-converters.R:reduceBigSpectronautvalidatesblock_size: rejects negative, zero,NA, length-2 vector, and unparseable string inputs.bigSpectronauttoMSstatsFormatplumbsblock_sizethrough: spies onreduceBigSpectronautviamockery::stub, asserts the default forwards16L * 1024L * 1024Land an explicit override forwards the user's value.cleanSpectronautChunkschema smoke test: synthetic minimal Spectronaut-shaped input, asserts output column set and basic values.cleanSpectronautChunkfilter_by_excluded: rows withExcluded == "True"getIntensity = NA.cleanSpectronautChunkfilter_by_identified: rows withIdentified == "False"getIntensity = NA.cleanSpectronautChunkfilter_by_qvalue(incl. NA case): rows below cutoff are kept, above cutoff becomeNA, andNAq-values becomeNA— the explicit semantic guarantee from the rewrite.cleanSpectronautChunkdrops rows whereF.FrgLossType != "noloss".Before this branch
cleanSpectronautChunkhad no direct test coverage (existing tests stubbedreduceBigSpectronautout entirely).Checklist Before Requesting a Review
Motivation & Context
Large Spectronaut exports previously used
readr::read_delim_chunked()for chunked CSV ingestion, which could lead to unbounded memory growth. Additionally, some wide exports hit Apache Arrow CSV “straddling object” parsing errors across block boundaries. Finally, the chunk cleaning logic relied on dplyr-style transformations rather than efficient in-place updates for high-volume streaming.This PR switches Spectronaut ingestion in
reduceBigSpectronaut()to an Arrow streaming pipeline (open_dataset()+Scanner+ToRecordBatchReader) with bounded peak memory, introduces a tunableblock_sizeto mitigate straddling-object failures, and rewritescleanSpectronautChunk()usingdata.tableto preserve existing filtering/NA semantics while improving performance.Detailed changes
Package metadata / dependencies
data.tabletoDESCRIPTIONimports.NAMESPACEto import requireddata.tablesymbols (:=,.SD,setDT,setnames) andstats::setNames.Arrow-based streaming ingestion with configurable chunk sizing
reduceBigSpectronaut(..., anomalyModelFeatures=c(), block_size = 16L * 1024L * 1024L):block_sizeis a single, non-NA, positive value (stopifnot(length(block_size) == 1L, !is.na(block_size), block_size > 0L)after coercion to integer).CsvReadOptions$create(block_size = block_size)to control parsing block size.Scannerwith a projection limited to only the columns needed by downstream cleaning (intersected with columns actually present).ToRecordBatchReader()/read_next_batch().cleanSpectronautChunk(..., pos=pos, ...), advancesposby the number of rows, and logs progress every 1,000 batches plus a final completion log.Chunk cleaning rewritten for efficient in-place processing
cleanSpectronautChunk()usingdata.table:data.table(setDT(input)) and filters to columns actually present in the batch.Intensityto numeric."True"string fields to logicals forExcludedand (when present and character-typed)Identified.filter_by_excluded: excluded rows getIntensity := NA_real_.filter_by_identified: unidentified rows getIntensity := NA_real_.filter_by_qvalue: requiresEGQvalue/PGQvalue, and preserves dplyr-likeif_elsesemantics by settingIntensity := NA_real_whenis.na(q) | q >= qvalue_cutoff.FFrgLossType == "noloss".IsotopeLabelTypefromLabeledSequence(H for Lys8/Arg10-containing sequences; otherwise L).Forwarding
block_sizefrom the public converterbigSpectronauttoMSstatsFormat(..., block_size = 16L * 1024L * 1024L):block_sizeand the “straddling object straddles two block boundaries” mitigation/tuning guidance.block_sizetoreduceBigSpectronaut().Documentation
man/bigSpectronauttoMSstatsFormat.Rdusage, parameter docs forblock_size, troubleshooting guidance for straddling-object errors, and details of required/optional Spectronaut export columns.man/dot-prefixedPath.Rdfor internal.prefixedPath(prefix, path).Unit tests added or modified
tests/testthat/test-converters.RreduceBigSpectronaut():block_sizevalidation rejects negative values, zero,NA, vectors, and non-numeric strings.cleanSpectronautChunk()runs.bigSpectronauttoMSstatsFormat():block_sizeis forwarded toreduceBigSpectronaut()(default and override).cleanSpectronautChunk():filter_by_excludedandfilter_by_identifiedsetIntensitytoNAappropriately.filter_by_qvalueNA-aware behavior matches dplyr-style semantics (NA q-values result inNAintensity).FFrgLossType == "noloss"are dropped.EG.Qvalue) are missing.Coding guidelines / violations
No coding guideline violations identified.