-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove file_type()
from FileFormat
#10499
Conversation
There are some related issues that I think might be good to close now? #8345 and #8637 I was able to implement basic read support for ORC files via ListingTable provider: https://github.com/datafusion-contrib/datafusion-orc/blob/main/src/datafusion/mod.rs Which might be enough to demonstrate those issues are done? |
Makes sense to me -- thank you @Jefffrey As you go through implementing ORC support, if you hit anything else that woudl make it easier to add new format support to the core and/or listing table that would be great. |
strictly speaking this is an API change but I think it makes it easier to implement new formats 🚀 |
Indeed -- it sounds reasonable In my mind, the ideal example is to make an example in I wonder if we could create a custom CSV reader that ignores comments 🤔 -- for example #10262 that @bbannier has been working on |
Will definitely keep this in mind 👍 |
Thanks again @Jefffrey |
Which issue does this PR close?
Closes #8657
Rationale for this change
Was implementing rudimentary ORC integration for DataFusion:
https://github.com/datafusion-contrib/datafusion-orc/blob/main/src/datafusion/file_format.rs
Noticed that to implement
FileFormat
it requires afile_type()
, even though this is not really used anywhere. Removing this can make it clearer for those wanting to extend with more custom file formats.What changes are included in this PR?
Remove
FileFormat::file_type()
which was unused.Are these changes tested?
Are there any user-facing changes?
Yes, function removed from trait