Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AdamGS
Copy link
Contributor

@AdamGS AdamGS commented Feb 20, 2025

Which issue does this PR close?

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?

@github-actions github-actions bot added the core Core DataFusion crate label Feb 20, 2025
@AdamGS AdamGS force-pushed the adamg/simplify-create-file-opener branch from eb80ee3 to c049a69 Compare February 20, 2025 16:39
@AdamGS AdamGS force-pushed the adamg/simplify-create-file-opener branch from c049a69 to 63a73e5 Compare February 20, 2025 16:48
@AdamGS AdamGS changed the title simplify fn signature Simplify FileSource::create_file_opener's signature Feb 20, 2025
@AdamGS
Copy link
Contributor Author

AdamGS commented Feb 20, 2025

After some thought, I think removing the Result from the return value is also valuable here, if I'm making a breaking change to the function (with its one callsite), might as well have two, and ideally they can both be merged as part of #14123.

If we want to add more validation to parameters, IMO the place to add that is when we use FileScanConfig to configure the source.

@alamb alamb added the api change Changes the API exposed to users of the crate label Feb 21, 2025
Copy link
Contributor

@alamb alamb left a 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>,
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileSource::create_file_opener takes a Result argument that is never used
2 participants