diff --git a/.gitignore b/.gitignore index 810a63b5f..547d6a4a5 100644 --- a/.gitignore +++ b/.gitignore @@ -16,6 +16,7 @@ benchmarks/artifacts/ /deps .vscode/ +.helix/ # Spark jar for iceberg data generation *.jar diff --git a/crates/glaredb/src/commands.rs b/crates/glaredb/src/commands.rs index 8adab294e..e89955ff4 100644 --- a/crates/glaredb/src/commands.rs +++ b/crates/glaredb/src/commands.rs @@ -252,7 +252,7 @@ impl RunCommand for MetastoreArgs { (Some(bucket), Some(service_account_path), None) => { let service_account_key = std::fs::read_to_string(service_account_path)?; StorageConfig::Gcs { - bucket: Some(bucket), + bucket, service_account_key, } } diff --git a/crates/object_store_util/src/conf.rs b/crates/object_store_util/src/conf.rs index 60044a057..ff4979f59 100644 --- a/crates/object_store_util/src/conf.rs +++ b/crates/object_store_util/src/conf.rs @@ -18,16 +18,16 @@ pub enum StorageConfig { secret_access_key: String, region: Option, endpoint: Option, - bucket: Option, + bucket: String, }, Gcs { service_account_key: String, - bucket: Option, + bucket: String, }, Azure { account_name: String, access_key: String, - container_name: Option, + container_name: String, }, Local { path: PathBuf, @@ -49,6 +49,7 @@ impl StorageConfig { let mut builder = AmazonS3Builder::new() .with_access_key_id(access_key_id) .with_secret_access_key(secret_access_key) + .with_bucket_name(bucket) .with_region(region.clone().unwrap_or_default()); if let Some(endpoint) = endpoint { @@ -66,22 +67,15 @@ impl StorageConfig { } } - if let Some(bucket) = bucket { - builder = builder.with_bucket_name(bucket); - } - Arc::new(builder.build()?) } StorageConfig::Gcs { service_account_key, bucket, } => { - let mut builder = - GoogleCloudStorageBuilder::new().with_service_account_key(service_account_key); - - if let Some(bucket) = bucket { - builder = builder.with_bucket_name(bucket); - } + let builder = GoogleCloudStorageBuilder::new() + .with_service_account_key(service_account_key) + .with_bucket_name(bucket); Arc::new(builder.build()?) } @@ -90,13 +84,10 @@ impl StorageConfig { access_key, container_name, } => { - let mut builder = MicrosoftAzureBuilder::new() + let builder = MicrosoftAzureBuilder::new() .with_account(account_name) - .with_access_key(access_key); - - if let Some(container_name) = container_name { - builder = builder.with_container_name(container_name); - } + .with_access_key(access_key) + .with_container_name(container_name); Arc::new(builder.build()?) } diff --git a/crates/sqlexec/src/engine.rs b/crates/sqlexec/src/engine.rs index 7fcf9e7b1..7bf5481dc 100644 --- a/crates/sqlexec/src/engine.rs +++ b/crates/sqlexec/src/engine.rs @@ -114,7 +114,8 @@ impl EngineStorageConfig { let bucket = opts .get("bucket") .cloned() - .or(url.host_str().map(|h| h.to_string())); + .or(url.host_str().map(|h| h.to_string())) + .ok_or(ExecError::MissingBucketName)?; EngineStorageConfig { location: url.clone(), @@ -156,6 +157,8 @@ impl EngineStorageConfig { } } + let bucket = bucket.ok_or(ExecError::MissingBucketName)?; + EngineStorageConfig { location: url.clone(), conf: StorageConfig::S3 { @@ -186,7 +189,8 @@ impl EngineStorageConfig { let container_name = opts .get("container_name") .cloned() - .or(url.host_str().map(|h| h.to_string())); + .or(url.host_str().map(|h| h.to_string())) + .ok_or(ExecError::MissingBucketName)?; EngineStorageConfig { location: url.clone(), @@ -227,21 +231,15 @@ impl EngineStorageConfig { .map_err(DatasourceCommonError::from)?, conf: StorageConfig::Gcs { service_account_key, - bucket: Some(bucket), + bucket, }, } } - // Expected gcs config opts for the session. - (StorageConfig::Gcs { bucket: None, .. }, None) => { - return Err(ExecError::InvalidStorageConfig( - "Missing bucket on session configuration", - )) - } (_, Some(_)) => EngineStorageConfig { location: Url::parse("memory://").map_err(DatasourceCommonError::from)?, conf: StorageConfig::Memory, }, - _ => self.clone(), + (_, None) => self.clone(), }) } @@ -567,14 +565,14 @@ mod tests { secret_access_key, region: None, endpoint: None, - bucket: Some("some-bucket".to_string()), + bucket: "some-bucket".to_string(), } ); let merged_conf = conf.with_session_config(&SessionStorageConfig { gcs_bucket: Some("my-other-bucket".to_string()), })?; - assert_eq!(merged_conf.conf, StorageConfig::Memory,); + assert_eq!(merged_conf.conf, StorageConfig::Memory); Ok(()) } } diff --git a/crates/sqlexec/src/errors.rs b/crates/sqlexec/src/errors.rs index 6b48da547..93bca702f 100644 --- a/crates/sqlexec/src/errors.rs +++ b/crates/sqlexec/src/errors.rs @@ -124,6 +124,9 @@ pub enum ExecError { #[error("Invalid URL for remote execution: {0}")] InvalidRemoteExecUrl(String), + #[error("Missing bucket name: required for storage configuration")] + MissingBucketName, + #[error(transparent)] DatasourceDebug(#[from] datasources::debug::errors::DebugError),