martin-g commented on code in PR #2803:
URL: https://github.com/apache/datafusion-comet/pull/2803#discussion_r2544529966
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -1852,14 +1869,95 @@ mod tests {
);
}
+ #[test]
+ fn test_extract_s3_config_path_style_access() {
+ // Case 1: path.style.access is true -> VirtualHostedStyleRequest
should be false
+ let mut configs = HashMap::new();
+ configs.insert("fs.s3a.path.style.access".to_string(),
"true".to_string());
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"false".to_string())
+ );
+
+ // Case 2: path.style.access is false -> VirtualHostedStyleRequest
should be true
+ let mut configs = HashMap::new();
+ configs.insert("fs.s3a.path.style.access".to_string(),
"false".to_string());
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"true".to_string())
+ );
+
+ // Case 3: path.style.access is not set -> VirtualHostedStyleRequest
should not be set
+ let configs = HashMap::new();
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ None
+ );
+ }
+
+
+ #[test]
+ fn test_normalize_endpoint() {
+ let bucket = "test-bucket";
+
+ // Case 1: force_path_style = true -> should append bucket
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000";, bucket, true),
+ Some("http://localhost:9000/test-bucket".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000/";, bucket, true),
+ Some("http://localhost:9000/test-bucket".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://my-s3-compatible.com";, bucket, true),
+ Some("https://my-s3-compatible.com/test-bucket".to_string())
+ );
+
+ // Case 2: force_path_style = false -> should insert bucket as
subdomain
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000";, bucket, false),
+ Some("http://test-bucket.localhost:9000".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://my-s3-compatible.com";, bucket, false),
+ Some("https://test-bucket.my-s3-compatible.com".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://s3.oss-me-central-1.aliyuncs.com";,
bucket, false),
+
Some("https://test-bucket.s3.oss-me-central-1.aliyuncs.com".to_string())
+ );
+
+ // Case 3: Default S3 endpoint -> should return None (ignored)
+ assert_eq!(
+ normalize_endpoint("s3.amazonaws.com", bucket, true),
+ None
+ );
+ assert_eq!(
+ normalize_endpoint("s3.amazonaws.com", bucket, false),
+ None
+ );
+
+ // Case 4: Empty endpoint -> should return None
+ assert_eq!(
+ normalize_endpoint("", bucket, true),
+ None
+ );
+ }
+
#[test]
fn test_extract_s3_config_custom_endpoint() {
+ // When path.style.access is not set, it defaults to false
(virtual-hosted-style)
Review Comment:
But
https://github.com/apache/datafusion-comet/pull/2803/files#diff-e8053ba6b0c33f3c35806e207a14881282d9c8a83c2a8adeed55d6a91493fb78R1892
says `Case 3: path.style.access is not set -> VirtualHostedStyleRequest should
not be set`
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -1852,14 +1869,95 @@ mod tests {
);
}
+ #[test]
+ fn test_extract_s3_config_path_style_access() {
+ // Case 1: path.style.access is true -> VirtualHostedStyleRequest
should be false
+ let mut configs = HashMap::new();
+ configs.insert("fs.s3a.path.style.access".to_string(),
"true".to_string());
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"false".to_string())
+ );
+
+ // Case 2: path.style.access is false -> VirtualHostedStyleRequest
should be true
+ let mut configs = HashMap::new();
+ configs.insert("fs.s3a.path.style.access".to_string(),
"false".to_string());
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ Some(&"true".to_string())
+ );
+
+ // Case 3: path.style.access is not set -> VirtualHostedStyleRequest
should not be set
+ let configs = HashMap::new();
+ let s3_configs = extract_s3_config_options(&configs, "test-bucket");
+ assert_eq!(
+ s3_configs.get(&AmazonS3ConfigKey::VirtualHostedStyleRequest),
+ None
+ );
+ }
+
+
+ #[test]
+ fn test_normalize_endpoint() {
+ let bucket = "test-bucket";
+
+ // Case 1: force_path_style = true -> should append bucket
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000";, bucket, true),
+ Some("http://localhost:9000/test-bucket".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000/";, bucket, true),
+ Some("http://localhost:9000/test-bucket".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://my-s3-compatible.com";, bucket, true),
+ Some("https://my-s3-compatible.com/test-bucket".to_string())
+ );
+
+ // Case 2: force_path_style = false -> should insert bucket as
subdomain
+ assert_eq!(
+ normalize_endpoint("http://localhost:9000";, bucket, false),
+ Some("http://test-bucket.localhost:9000".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://my-s3-compatible.com";, bucket, false),
+ Some("https://test-bucket.my-s3-compatible.com".to_string())
+ );
+ assert_eq!(
+ normalize_endpoint("https://s3.oss-me-central-1.aliyuncs.com";,
bucket, false),
+
Some("https://test-bucket.s3.oss-me-central-1.aliyuncs.com".to_string())
+ );
+
+ // Case 3: Default S3 endpoint -> should return None (ignored)
+ assert_eq!(
+ normalize_endpoint("s3.amazonaws.com", bucket, true),
+ None
+ );
+ assert_eq!(
+ normalize_endpoint("s3.amazonaws.com", bucket, false),
+ None
+ );
+
+ // Case 4: Empty endpoint -> should return None
+ assert_eq!(
+ normalize_endpoint("", bucket, true),
+ None
+ );
+ }
+
#[test]
fn test_extract_s3_config_custom_endpoint() {
+ // When path.style.access is not set, it defaults to false
(virtual-hosted-style)
Review Comment:
```suggestion
// When path.style.access is not set, it defaults to false (i.e.
virtual-hosted-style=true)
```
Is this what you mean ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
