feat: Add substrait support for multiple files#19729
feat: Add substrait support for multiple files#19729kumarUjjawal wants to merge 6 commits intoapache:mainfrom
Conversation
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
Dropping in from the substrait side of things 👋
Just had a chance to take a quick look, hopefully I can give a more fully review later
| // 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 | ||
| }; |
There was a problem hiding this 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.
There was a problem hiding this 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, we may use the scheme to determine if we actually need to read the file from e.g. s3.
There was a problem hiding this comment.
Thanks for the feedback. I had few thoughts:
The Substrait spec says these are "URIs" where "the protocol portion of the URI determines the storage type", but the existing physical plan producer/consumer in DF treats them as raw strings, the producer emits bare object-store paths like "file1.parquet" (not valid URIs), and the consumer passes them straight to ObjectMeta without parsing. If we require Url::parse() to succeed, plans produced by DataFusion's own physical plan producer would be rejected by our logical plan consumer.
So I was thinking we keep &[String] and pass raw URI strings and let the consumer decide how to parse. consistent with the physical plan side. What do you think?
There was a problem hiding this comment.
Hmm to me that sounds like a bug on the consumer side too then. We should only produce plans with valid URIs IMO.
I'm not the datafusion expert, so I'm not sure the right approach here but I feel like it would be better to fix that bug rather than allow the incorrect substrait in the producer require accommodation in all of the consumer parts of the code.
There was a problem hiding this comment.
I looked into the producer and what i can understand, it appears to emit relative object-store paths by design (the scheme lives in ObjectStoreUrl separately). For our logical plan consumer, we'll require valid URIs per spec. I have made the changes here for the URI.
benbellick
left a comment
benbellick
left a comment
There was a problem hiding this comment.
On the whole I think it looks good, but I have a few remaining questions. The biggest one is about whether we should just let the failure to handle multiple local files just percolate up to an execution error.
I suspect that returning an error there is better than returning a potentially incorrect query result.
| .map(|filename| filename.to_string_lossy().to_string()) | ||
| /// Parses the URI string from a PathType variant. | ||
| /// Returns an error if the URI is malformed. | ||
| fn parse_uri( |
There was a problem hiding this comment.
Just a note in case someone comes to this in the future and as the same question.
The Url crate is quite permissive, so while the type is called Url, it handles general URIs without a problem.
| // Try to resolve files using the new trait method. | ||
| // If not implemented, fall back to the legacy single-file behavior | ||
| // for backward compatibility. | ||
| let provider = match consumer |
There was a problem hiding this comment.
This is also a question for the people with more datafusion experience, but is this the right approach? I would think that if the consumer fails to handle multiple files, it is an execution failure. Otherwise, doesn't it risk executing the plan incorrectly?
There was a problem hiding this comment.
We should probably propagate error in multi-file case.
|
|
||
| if lf.items.len() > 1 || filename.is_none() { | ||
| return not_impl_err!("Only single file reads are supported"); | ||
| if uris.is_empty() { |
There was a problem hiding this comment.
One other small thing. I don't think the spec technically enforces that an empty LocalFiles is invalid. Since the schema will have to be part of the plan, it is also reasonable to just consider it an empty table 🤷
I don't know what is consistent with the normal datafusion approach here.
There was a problem hiding this comment.
In that way, I see no reason to not just let the implementer of resolve_local_files handle this case. The only complexity then is that if you don't have any uris, then you can't make a table name.
kumarUjjawal
force-pushed
the
feat/multi_file_substrait
branch
from
487c468 to
206e05d
Compare
2 participants
Which issue does this PR close?
Rationale for this change
In #10842 support for
ReadType::LocalFileswas added but it didn't support multiple files.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?