You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "benbellick (via GitHub)" <gi...@apache.org> on 2026/02/24 16:26:15 UTC
Re: [PR] feat: Add substrait support for multiple files [datafusion]
benbellick commented on code in PR #19729:
URL: https://github.com/apache/datafusion/pull/19729#discussion_r2848212825
##########
datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs:
##########
@@ -176,45 +176,100 @@ pub async fn from_read_rel(
}))
}
Some(ReadType::LocalFiles(lf)) => {
- fn extract_filename(name: &str) -> Option<String> {
- let corrected_url =
- if name.starts_with("file://") && !name.starts_with("file:///") {
- name.replacen("file://", "file:///", 1)
- } else {
- name.to_string()
- };
-
- Url::parse(&corrected_url).ok().and_then(|url| {
- let path = url.path();
- std::path::Path::new(path)
- .file_name()
- .map(|filename| filename.to_string_lossy().to_string())
- })
+ /// Extracts the path string from a PathType variant.
+ /// Normalizes file:// URLs to file:/// format for proper URL parsing.
+ fn extract_path(path_type: Option<&PathType>) -> Option<String> {
+ let path_str = match path_type? {
+ PathType::UriPath(p) => p.clone(),
+ PathType::UriPathGlob(p) => p.clone(),
+ PathType::UriFile(p) => p.clone(),
+ PathType::UriFolder(p) => p.clone(),
+ };
+
+ // Normalize file:// to file:/// for proper URL parsing
+ let normalized = if path_str.starts_with("file://")
+ && !path_str.starts_with("file:///")
+ {
+ path_str.replacen("file://", "file:///", 1)
+ } else {
+ path_str
+ };
Review Comment:
Furthermore, shouldn't we pass around the `Url` instead of a string file path? We may need the scheme later. [Per the substrait spec](https://github.com/substrait-io/substrait/blob/7636cee7e5ebde47384e2da13c44f840b3fc288c/proto/substrait/algebra.proto#L176-L178), we may use the scheme to determine if we actually need to read the file from e.g. `s3`.
##########
datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs:
##########
@@ -176,45 +176,100 @@ pub async fn from_read_rel(
}))
}
Some(ReadType::LocalFiles(lf)) => {
- fn extract_filename(name: &str) -> Option<String> {
- let corrected_url =
- if name.starts_with("file://") && !name.starts_with("file:///") {
- name.replacen("file://", "file:///", 1)
- } else {
- name.to_string()
- };
-
- Url::parse(&corrected_url).ok().and_then(|url| {
- let path = url.path();
- std::path::Path::new(path)
- .file_name()
- .map(|filename| filename.to_string_lossy().to_string())
- })
+ /// Extracts the path string from a PathType variant.
+ /// Normalizes file:// URLs to file:/// format for proper URL parsing.
+ fn extract_path(path_type: Option<&PathType>) -> Option<String> {
+ let path_str = match path_type? {
+ PathType::UriPath(p) => p.clone(),
+ PathType::UriPathGlob(p) => p.clone(),
+ PathType::UriFile(p) => p.clone(),
+ PathType::UriFolder(p) => p.clone(),
+ };
+
+ // Normalize file:// to file:/// for proper URL parsing
+ let normalized = if path_str.starts_with("file://")
+ && !path_str.starts_with("file:///")
+ {
+ path_str.replacen("file://", "file:///", 1)
+ } else {
+ path_str
+ };
Review Comment:
Just a question, I realize that this is consistent with the previous code, but why is it the responsibility of this point in the code to "normalize" the url?
I put normalize in quotes because aren't we actually just taking an incorrect url and modifying it to be correct? I would consider that malformed substrait and throw an error personally.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org