-
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
Simplify FileSource::create_file_opener
's signature
#14798
base: main
Are you sure you want to change the base?
Conversation
eb80ee3
to
c049a69
Compare
c049a69
to
63a73e5
Compare
FileSource::create_file_opener
's signature
After some thought, I think removing the If we want to add more validation to parameters, IMO the place to add that is when we use |
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 @AdamGS
I agree this is an API change, but in my opinion it is for the better and will not cause undue issues during upgrade
However, let's leave it open for a few days to give others a chane to weigh in
@@ -38,10 +38,10 @@ pub trait FileSource: Send + Sync { | |||
/// Creates a `dyn FileOpener` based on given parameters | |||
fn create_file_opener( | |||
&self, | |||
object_store: datafusion_common::Result<Arc<dyn ObjectStore>>, | |||
object_store: Arc<dyn ObjectStore>, |
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.
I agree the old signature is very weird
Which issue does this PR close?
FileSource::create_file_opener
takes aResult
argument that is never used #14797 .Rationale for this change
Makes the interface slightly more idiomatic.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?