-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
use FileFormat::get_ext as the default file extension filter #12417
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @waruto210 -- this makes sense to me and I think is a nice improvement.
I have a small API improvement suggestion, but I think we can do it as a follow on PR if you prefer.
Thanks again!
) -> Result<()> { | ||
let ctx = SessionContext::new(); | ||
register_test_store(&ctx, &files.iter().map(|f| (*f, 10)).collect::<Vec<_>>()); | ||
|
||
let format = AvroFormat {}; | ||
|
||
let opt = ListingOptions::new(Arc::new(format)) | ||
.with_file_extension("") | ||
let mut opt = ListingOptions::new(Arc::new(format)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make this a nicer API by adding with_file_extension_opt
or something so this code could be like:
let opt = ListingOptions::new(Arc::new(format))
.with_file_extension_opt(file_ext)
.with_target_partitions(target_partitions);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make this a nicer API by adding
with_file_extension_opt
or something so this code could be like:let opt = ListingOptions::new(Arc::new(format)) .with_file_extension_opt(file_ext) .with_target_partitions(target_partitions);
I think your advice is great, I'm on vacation right now and I'll revise the code when I get back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a PR with the proposed API: #12461
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a PR with the proposed API: #12461
I have modified the code according to this API.
BTW I am holding off on merging this PR until we cut the 42 release candidate to avoid introducing last minute behavior changes |
022c8f6
to
ad55fdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @waruto210 -- this is great. 🚀
Which issue does this PR close?
Closes #12378.
Rationale for this change
Using
FileFormat::get_ext
as the default file extension filter better meets user expectations.What changes are included in this PR?
Use
FileFormat::get_ext
as the default file extension filterAre these changes tested?
Yes
Are there any user-facing changes?
User do not need to specify the file extension if they choose the default value.