ozankabak commented on code in PR #4525:
URL: https://github.com/apache/arrow-datafusion/pull/4525#discussion_r1041076804
##########
datafusion/core/src/datasource/listing/url.rs:
##########
@@ -99,10 +99,14 @@ impl ListingTableUrl {
};
let path = std::path::Path::new(prefix).canonicalize()?;
- let url = match path.is_file() {
- true => Url::from_file_path(path).unwrap(),
- false => Url::from_directory_path(path).unwrap(),
- };
+ let url = if path.is_dir() {
+ Url::from_directory_path(path)
+ } else {
+ Url::from_file_path(path)
+ }
+ .map_err(|_| DataFusionError::Internal(format!("Can not open path:
{}", s)))?;
Review Comment:
This is a generalization of the old checking logic. When you check with
`is_file()`, you can not read certain file types such as FIFO files. To support
all kinds of readable files, one typically checks with `!is_dir()`. You can get
more context [here at this clippy
discussion](https://github.com/rust-lang/rust-clippy/issues/4503).
So this change is simply applying the modification above and turning the
boolean `match` expression to an `if` expression, which is slightly more
idiomatic.
Are you asking about `map_err` vs `unwrap`? We weren't sure if an error is
impossible here, so we added the `map_err` just in case. I typically prefer
doing this for the purposes of defensive programming:
- If what is now impossible becomes possible in the future because the code
calling `canonicalize` changes for any reason, we would still be producing a
sensible error instead of panicking.
- It also guards against cases where the path is valid at the time you call
`canonicalize`, but then gets in between two function calls. This is not a
realistic case, but it is possible in theory.
However, if you believe it reduces readability, we can go back to `unwrap`.
--
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]
