Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rust): special characters in s3 paths #2308

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

alexr-bq
Copy link

Description

Fixes #2266

We construct a "Prefix handler" for S3 paths. This prefix is passed into an s3 API that does not expect a URL-encoded path, so here we decode the path to support paths with characters such as spaces.

Related Issue(s)

2266

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Mar 20, 2024
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@alexr-bq alexr-bq changed the title Special char fix fix(rust) Special char fix Mar 20, 2024
@alexr-bq alexr-bq changed the title fix(rust) Special char fix fix(rust): Special char fix Mar 20, 2024
@alexr-bq alexr-bq changed the title fix(rust): Special char fix fix(rust): special characters in s3 paths Mar 20, 2024
crates/aws/src/lib.rs Outdated Show resolved Hide resolved
@@ -46,7 +46,18 @@ impl LogStoreFactory for S3LogStoreFactory {
location: &Url,
options: &StorageOptions,
) -> DeltaResult<Arc<dyn LogStore>> {
let store = url_prefix_handler(store, Path::parse(location.path())?)?;
let path_decoded = match urlencoding::decode(location.path()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line would benefit from a comment as to what this decoding does/why it exists

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Member

@rtyler rtyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would feel more confident with a test, but I'm willing to roll the dice here 😄 🎲

@jkylling
Copy link
Contributor

Is it correct to use URL encoding here? I believe Delta Lake uses URI encoded paths everywhere and not URL encoded paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to read from table in S3 with special characters like spaces in path
3 participants