Features/chunked nc reader - #98
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for chunked reading of netCDF files, enabling streaming access to large datasets by reading data in configurable chunks rather than loading entire files into memory at once.
- Introduces a new
Streamstruct that implements both synchronous and asynchronous iteration over netCDF data chunks - Adds flexible chunking strategies including automatic balancing, custom chunk sizes, and no chunking
- Refactors the existing reader to use the new streaming infrastructure while maintaining backward compatibility
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| beacon-arrow-netcdf/src/reader.rs | Refactored to use Arc for thread safety, added streaming methods, and updated variable reading to support chunked access |
| beacon-arrow-netcdf/src/chunked_stream.rs | New module implementing the core streaming functionality with chunking logic and dimension validation |
| beacon-arrow-netcdf/src/nc_array.rs | Added convenience constructor for Dimension struct |
| beacon-arrow-netcdf/src/lib.rs | Added chunked_stream module export |
| beacon-arrow-netcdf/src/error.rs | Added Stream and Reader error variants |
| beacon-arrow-netcdf/Cargo.toml | Added futures and serde dependencies |
| beacon-api/Cargo.toml | Fixed jemalloc dependency to be platform-specific |
| Cargo.toml | Added derive feature to serde and serde feature to indexmap |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| file.dimensions().try_for_each(|dim| { | ||
| if dimensions.contains(&dim.name().to_string()) { | ||
| Ok(()) | ||
| } else { | ||
| Err(ArrowNetCDFError::Reader(format!( | ||
| "Dimension '{}' not found in NetCDF file.", | ||
| dim.name() | ||
| ))) | ||
| } | ||
| })?; |
There was a problem hiding this comment.
The logic is inverted - this should check if the provided dimensions exist in the file, not if file dimensions exist in the provided list. Currently it will error when the file has dimensions not in the input list, rather than when input dimensions don't exist in the file.
| file.dimensions().try_for_each(|dim| { | |
| if dimensions.contains(&dim.name().to_string()) { | |
| Ok(()) | |
| } else { | |
| Err(ArrowNetCDFError::Reader(format!( | |
| "Dimension '{}' not found in NetCDF file.", | |
| dim.name() | |
| ))) | |
| } | |
| })?; | |
| let file_dimension_names: Vec<String> = file | |
| .dimensions() | |
| .map(|dim| dim.name().to_string()) | |
| .collect(); | |
| for dim in &dimensions { | |
| if !file_dimension_names.contains(dim) { | |
| return Err(ArrowNetCDFError::Reader(format!( | |
| "Provided dimension '{}' not found in NetCDF file.", | |
| dim | |
| ))); | |
| } | |
| } |
| // Also remove variable attributes if the parent variable is removed | ||
| || (field_name.contains('.') && { |
There was a problem hiding this comment.
The logic on line 84-89 is incorrect. The condition should retain fields that are NOT in removable_variables AND retain attribute fields whose parent variables are NOT removed. The current logic uses || where it should use &&, and the second condition has incorrect negation.
| // Also remove variable attributes if the parent variable is removed | |
| || (field_name.contains('.') && { | |
| // Also remove variable attributes only if the parent variable is not removed | |
| && (!field_name.contains('.') || { |
| { | ||
| fill_value = read_fill_value_attribute(&$var)?.and_then(|fv| fv.$as_ref()) | ||
| } |
There was a problem hiding this comment.
[nitpick] The braces around this single statement are unnecessary and reduce code readability.
| { | |
| fill_value = read_fill_value_attribute(&$var)?.and_then(|fv| fv.$as_ref()) | |
| } | |
| fill_value = read_fill_value_attribute(&$var)?.and_then(|fv| fv.$as_ref()) |
| println!( | ||
| "Reading variable {} with hyper slab: {:?}", | ||
| variable.name(), | ||
| hyper_slab | ||
| ); |
There was a problem hiding this comment.
Debug print statements should not be left in production code. Consider using a proper logging framework or removing this statement.
* init * wip chunked stream * wip chunked reader
Support chunked reading for netcdf files