fix: Correctly interpret and apply S3 path.style.access configurati…
#2803
Conversation
…on, including endpoint normalization and adding tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2803 +/- ##
============================================
+ Coverage 56.12% 58.31% +2.18%
- Complexity 976 1422 +446
============================================
Files 119 162 +43
Lines 11743 14145 +2402
Branches 2251 2363 +112
============================================
+ Hits 6591 8248 +1657
- Misses 4012 4704 +692
- Partials 1140 1193 +53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…stom endpoints by inserting the bucket as a subdomain and update related tests.
|
|
||
| #[test] | ||
| fn test_extract_s3_config_custom_endpoint() { | ||
| // When path.style.access is not set, it defaults to false (virtual-hosted-style) |
There was a problem hiding this comment.
| // When path.style.access is not set, it defaults to false (virtual-hosted-style) | |
| // When path.style.access is not set, it defaults to false (i.e. virtual-hosted-style=true) |
Is this what you mean ?
There was a problem hiding this 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
| s3_configs.insert(AmazonS3ConfigKey::Region, region.to_string()); | ||
| } | ||
|
|
||
| // Extract and handle path style access (virtual hosted style) |
There was a problem hiding this comment.
| // Extract and handle path style access (virtual hosted style) | |
| // Set 'virtual hosted style' config depending on the value of 'path style access' |
I am not sure that it is more clear
| 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()); |
There was a problem hiding this comment.
| configs.insert("fs.s3a.path.style.access".to_string(), "true".to_string()); | |
| configs.insert("fs.s3a.path.style.access".to_string(), "tRue".to_string()); |
to test path_style_access = path_style.to_lowercase() == "true"; (line 174)
Or maybe add a separate test for this.
| let rest = &endpoint[protocol_end + 3..]; // "s3.oss-me-central-1.aliyuncs.com" or "s3.oss-me-central-1.aliyuncs.com/path" | ||
|
|
||
| // Insert bucket as first subdomain | ||
| Some(format!("{protocol}{bucket}.{rest}")) |
There was a problem hiding this comment.
According to https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html#general-purpose-bucket-names
Bucket names can consist only of lowercase letters, numbers, periods (.), and hyphens (-).
usage of periods (.) in the bucket name would create several subdomains. Is that supported ? Or a check should be added and reported somehow ?!
3 participants
…on, including endpoint normalization and adding tests.
Which issue does this PR close?
Closes #2802
Rationale for this change
To proper consistency in creating s3 connection string
What changes are included in this PR?
fix for the path style config handling
How are these changes tested?
manual and test cases