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.

Sign up for GitHub

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 ListingTableUrl to decode percent #3750

Merged
merged 5 commits into from
Oct 11, 2022
Merged

Fix ListingTableUrl to decode percent #3750

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ee83dd
fix: ListingTabUrl prefix decoding
unvalley Oct 7, 2022
92ef8be
chore: remove waste change
unvalley Oct 7, 2022
65ba95e
fix: use from instead of parse
unvalley Oct 7, 2022
439f6dc
test: add test cases for prefix
unvalley Oct 7, 2022
7338da4
chore: cargo fmt
unvalley Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datafusion/core/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ ordered-float = "3.0"
parking_lot = "0.12"
parquet = { version = "24.0.0", features = ["arrow", "async"] }
paste = "^1.0"
percent-encoding = "2.2.0"
Copy link
Contributor Author

@unvalley unvalley Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

datafusion/core uses url crate by servo.
percent-encoding is also published by the same repository.

pin-project-lite = "^0.2.7"
pyo3 = { version = "0.17.1", optional = true }
rand = "0.8"
Expand Down
7 changes: 6 additions & 1 deletion datafusion/core/src/datasource/listing/url.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use itertools::Itertools;
use object_store::path::Path;
use object_store::{ObjectMeta, ObjectStore};
use url::Url;
use percent_encoding;

/// A parsed URL identifying files for a listing table, see [`ListingTableUrl::parse`]
/// for more information on the supported expressions
Expand Down Expand Up @@ -108,7 +109,8 @@ impl ListingTableUrl {

/// Creates a new [`ListingTableUrl`] from a url and optional glob expression
fn new(url: Url, glob: Option<Pattern>) -> Self {
let prefix = Path::parse(url.path()).expect("should be URL safe");
let decoded_path = percent_encoding::percent_decode_str(url.path()).decode_utf8_lossy();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the url.path(), it is still percent encoded string.
percent_decode_str decodes it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will cause the parse call below to fail in the event of non-ASCII characters.

I think it should just be a case of change Path::parse to Path::from

let prefix = Path::parse(decoded_path).expect("should be URL safe");
Self { url, prefix, glob }
}

Expand Down Expand Up @@ -246,6 +248,9 @@ mod tests {
let url = ListingTableUrl::parse("file:///foo").unwrap();
let child = Path::parse("/foob/bar").unwrap();
assert!(url.strip_prefix(&child).is_none());

let url = ListingTableUrl::parse("file:///with space/foo/bar").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get a test of a non-ASCII character such as a percent encoded emoji or something

We should also test non-URL safe characters like a percent encoded ?

Copy link
Contributor Author

@unvalley unvalley Oct 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I added some test cases at 439f6dc
if it's not enough or wrong to test, please tell me about it.

assert_eq!(url.prefix.as_ref(), "with space/foo/bar");
}

#[test]
Expand Down