Storage config owns data dirs + optional S3; NetCDF output to configured tmp - #291
Merged
Merged
Conversation
StorageConfig now owns the local storage layout (data_dir/datasets_dir/ tables_dir/tmp_dir), so ObjectStores::new(&StorageConfig) and the iceberg warehouse take a single config instead of loose path args re-derived at each call site. This removes the multi-source temp-dir coupling structurally. S3 is now Option<S3Config>: presence is the backend switch (None = local, Some = S3), replacing the separate data_lake bool. Within S3Config the bucket is a required String (object_store's AmazonS3Builder requires it regardless of addressing style and never infers it from the endpoint); Config::load rejects an empty bucket when S3 is enabled. endpoint/region stay optional. beacon-config keeps indexes/cache in DataDirsConfig; the storage dirs moved to config.storage. Updated runtime, iceberg, data-lake, and the runtime_config test accordingly.
The NetCDF sinks hardcoded std::env::temp_dir() for the output file, so
`output: { format: netcdf }` ignored config.storage.tmp_dir and (after the
tmp-store move) wrote to a different directory than the returned file handle —
producing an empty download.
Thread the tmp directory through: expose DatasetsStore::storage(), and in
NetcdfFormat::create_writer_physical_plan read storage().tmp_dir (the factory
already holds the datasets store, which now carries the full StorageConfig) and
pass it to NetCDFSink/NetCDFNdSink, which write there instead of temp_dir().
Adds an end-to-end regression test asserting NetCDF output lands under the
configured tmp dir and is non-empty.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the storage-configuration consolidation by making StorageConfig the single owner of local directory layout and by switching the datasets backend selection to Option<S3Config>. It also fixes a NetCDF output regression where the sink wrote to the OS temp directory instead of the configured tmp directory, causing empty/incorrect downloads.
Changes:
- Move
data_dir/datasets_dir/tables_dir/tmp_dirintoStorageConfigand simplify object store initialization to take only&StorageConfig. - Make S3 optional (
Option<S3Config>) and requireS3Config.bucketwhen S3 is enabled (validated inConfig::load). - Thread the configured tmp directory into NetCDF sinks and add an end-to-end regression test ensuring NetCDF output is written under
config.storage.tmp_dir.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| beacon-object-storage/src/lib.rs | Simplifies ObjectStores::new to use directory layout from StorageConfig. |
| beacon-object-storage/src/datasets_store.rs | Switches S3 selection to Option<S3Config>, stores full StorageConfig, exposes DatasetsStore::storage(). |
| beacon-object-storage/src/config.rs | Expands StorageConfig to own local dirs; makes S3Config.bucket required and simplifies builder creation. |
| beacon-iceberg/src/catalog.rs | Uses Option<S3Config> and storage.datasets_dir for local Iceberg warehouse initialization. |
| beacon-file-formats/beacon-arrow-netcdf/src/datafusion/sink.rs | Updates NetCDF sinks to write into a provided output_dir instead of std::env::temp_dir(). |
| beacon-file-formats/beacon-arrow-netcdf/src/datafusion/mod.rs | Threads storage().tmp_dir into NetCDF sink construction. |
| beacon-data-lake/src/lib.rs | Updates tmp dir wiring to use config.storage.tmp_dir. |
| beacon-core/tests/runtime_config.rs | Updates tests to configure storage dirs via config.storage.* instead of config.data.*. |
| beacon-core/src/runtime.rs | Updates object store initialization + Iceberg init call; adds NetCDF tmp-dir regression test. |
| beacon-config/src/lib.rs | Constructs StorageConfig local dirs from BEACON_DATA_DIR; validates S3 bucket presence; creates dirs from config.storage.*. |
| beacon-config/src/error.rs | Adds ConfigError::InvalidStorage for inconsistent storage settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+88
to
94
| pub async fn local_datasets_store(datasets_dir: PathBuf) -> StorageResult<DatasetsStore> { | ||
| let storage = StorageConfig { | ||
| datasets_dir, | ||
| ..StorageConfig::default() | ||
| }; | ||
| create_datasets_store(&storage).await | ||
| } |
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
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.
Context
Follow-up to #290, which landed in
mainas the S3-single-source-of-truth + temp-output rooting work. This PR continues the storage-config cleanup and fixes a latent NetCDF output bug.Changes
1. Data dirs move into
StorageConfig; S3 becomes optionalStorageConfignow owns the local layout (data_dir/datasets_dir/tables_dir/tmp_dir), soObjectStores::new(&StorageConfig)and the iceberg warehouse take one config instead of loose path args re-derived per call site — removing the multi-source temp-dir coupling structurally.Option<S3Config>: presence is the backend switch (None= local,Some= S3), replacing the separatedata_lakebool.S3Config.bucketis a requiredString—object_store'sAmazonS3Builderrequires a bucket regardless of addressing style and never infers it from the endpoint;Config::loadrejects an empty bucket when S3 is enabled.endpoint/regionstay optional.indexes/cacheremain inbeacon-config'sDataDirsConfig.2. NetCDF output writes to the configured tmp dir
std::env::temp_dir(), sooutput: { format: netcdf }ignoredconfig.storage.tmp_dirand wrote to a different directory than the returned file handle — yielding an empty download.DatasetsStore::storage()is now exposed;NetcdfFormat::create_writer_physical_planreadsstorage().tmp_dir(the factory already holds the datasets store, which carries the fullStorageConfig) and passes it toNetCDFSink/NetCDFNdSink, which write there instead of the OS temp dir.Verification
cargo build --workspaceclean.cargo test --workspace --liball green.query_with_netcdf_output_writes_under_configured_tmpruns a real NetCDFCOPYand asserts the file lands under the configured tmp dir and is non-empty (failed before the fix, passes now).