M sstats big/work/20260513 arrow chunking followups#17
Open
Rudhik1904 wants to merge 12 commits into
Open
Conversation
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.
📝 WalkthroughWalkthroughThis PR switches Spectronaut conversion to Arrow-backed record-batch streaming, rewrites chunk cleaning with data.table, adds a configurable 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
| anomalyModelFeatures=c()) { | ||
| calculateAnomalyScores=FALSE, | ||
| anomalyModelFeatures=c(), | ||
| block_size = 16L * 1024L * 1024L) { |
Contributor
There was a problem hiding this comment.
- For people who may want to increase their block size to increase the speed of processing, add a recommendation on how to estimate the adequate block size to maximize speed while reducing the risk of the system crashing
- Make it more clear in the MSstatsBig / MSstatsConvert documentation on which columns we actually need from users for Spectronaut
Contributor
Author
There was a problem hiding this comment.
added the documentation, let me know how it looks if it needs more work
…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
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 relied on
readr::read_delim_chunked()for chunked CSV ingestion, which can retain growing per-chunk state and lead to unbounded memory growth. Additionally, some wide exports triggered Apache Arrow “straddling object” failures when CSV parsing happens across block boundaries. Finally, the chunk-cleaning logic (cleanSpectronautChunk()) used dplyr-style transformations, which are less efficient for high-volume, in-place chunk processing.Solution: switch Spectronaut CSV reading to an Arrow streaming, record-batch-by-record-batch pipeline (bounded peak memory) and introduce a tunable
block_sizeto mitigate straddling-object issues. ReworkcleanSpectronautChunk()to usedata.tablefor efficient in-place transformations while preserving the existing filtering/NA semantics.Detailed changes
Package metadata / imports
DESCRIPTION: addeddata.tabletoImports.NAMESPACE: addedimportFrom(data.table, ":="),importFrom(data.table, ".SD"),importFrom(data.table, setDT),importFrom(data.table, setnames).R/clean_spectronaut.RreduceBigSpectronaut(..., block_size = 16L * 1024L * 1024L):block_size(as.integer();stopifnot(length(block_size)==1L, !is.na(block_size), block_size > 0L)).csv→,,tsv|xls→ tab, else;).needed_cols(plusanomalyModelFeatureswhencalculateAnomalyScores=TRUE).arrow::open_dataset(..., format="csv", parse_options=CsvParseOptions$create(delimiter=delim), read_options=CsvReadOptions$create(block_size=block_size))arrow::Scanner$create(ds, projection=present_cols)wherepresent_cols <- intersect(needed_cols, names(ds))(tolerates partial exports).scanner$ToRecordBatchReader()and processesreader$read_next_batch()in a loop.data.frameand callscleanSpectronautChunk(..., pos=pos, ...).pos <- pos + nrow(chunk_df).cleanSpectronautChunk(input, output_path, ..., calculateAnomalyScores=FALSE, anomalyModelFeatures=c()):data.table:data.table::setDT(input)to operate in-place.present_orig <- intersect(all_cols, colnames(input)); input <- input[, present_orig, with=FALSE].MSstatsConvert:::.standardizeColnames().setnames(..., skip_absent = TRUE).Intensity := as.numeric(Intensity)."True"/character values forExcludedto logical whenExcludedis character.Identifiedto logical when present and character-typed.require_filter_cols()which stops with a clear message naming missing Spectronaut source columns when filter columns are absent.filter_by_excluded: setsIntensity := NA_real_whereExcluded == TRUE.filter_by_identified: setsIntensity := NA_real_whereIdentified == FALSE.filter_by_qvalue:EGQvalue/PGQvalue.dplyr::if_elsestyle): setsIntensity := NA_real_whenis.na(EGQvalue) | EGQvalue >= qvalue_cutoffand similarly forPGQvalue.FFrgLossTypeand filters rows withFFrgLossType == "noloss".LabeledSequenceexists:IsotopeLabelTypebecomes"H"whenLys8orArg10is detected, else"L"."L"..writeChunkToFile().R/converters.RbigSpectronauttoMSstatsFormat(..., block_size = 16L * 1024L * 1024L):block_sizeparameter (documented to help tune Arrow parsing / mitigate “straddling object” failures).block_sizetoreduceBigSpectronaut().Documentation
man/bigSpectronauttoMSstatsFormat.Rd: updated usage/signature and expandedblock_sizedocumentation and tuning guidance.man/dot-prefixedPath.Rd: added docs for internal.prefixedPath(prefix, path).Unit tests added or modified
tests/testthat/test-converters.RcleanSpectronautChunk():filter_by_excludedsetsIntensitytoNAfor excluded rows.filter_by_identifiedsetsIntensitytoNAfor unidentified rows.filter_by_qvalueverifies NA-aware behavior (rows with NA q-values become NA intensity).FFrgLossType != "noloss".F.FrgLossType/EG.Qvalue).filter_by_qvalue=TRUE.reduceBigSpectronaut():block_sizerejects invalid values (negative, zero,NA, vector, and non-numeric string).cleanSpectronautChunk()sees them.bigSpectronauttoMSstatsFormat():block_sizeis forwarded through toreduceBigSpectronaut()(default and override).Coding guidelines / violations
No coding guideline violations identified.