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

Make ObjectStoreScheme public #5912

Merged
merged 3 commits into from
Jun 30, 2024
Merged

Make ObjectStoreScheme public #5912

merged 3 commits into from
Jun 30, 2024

Conversation

orf
Copy link
Contributor

@orf orf commented Jun 18, 2024

Which issue does this PR close?

Closes #5911.

Rationale for this change

ObjectStoreScheme::parse contains non-trivial logic to resolve a given url into an object store scheme. Exposing this is useful, as it allows consumers to set up their own builders as they would like.

What changes are included in this PR?

This just makes the types public.

Are there any user-facing changes?

The types become public.

@github-actions github-actions bot added the object-store Object Store Interface label Jun 18, 2024
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.

Thanks @orf -- this makes sense to me

I think we should wait for @tustvold to weigh in on any reason it would is important to keep this implementation trait private.

We also have some duplicated version of this logic in DataFusion so having it in one central place would be quite nice

Copy link

@leoyvens leoyvens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wished for this, so thanks! I'm not a maintainer but hopefully this is a helpful review.

@@ -62,7 +62,7 @@ impl ObjectStoreScheme {
/// Create an [`ObjectStoreScheme`] from the provided [`Url`]
///
/// Returns the [`ObjectStoreScheme`] and the remaining [`Path`]
fn parse(url: &Url) -> Result<(Self, Path), Error> {
pub fn parse(url: &Url) -> Result<(Self, Path), Error> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to change this to return super::Error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in b9ca112 (by making error public not by returning super::Error)

@@ -43,7 +43,7 @@ impl From<Error> for super::Error {

/// Recognises various URL formats, identifying the relevant [`ObjectStore`]
#[derive(Debug, Eq, PartialEq)]
enum ObjectStoreScheme {
pub enum ObjectStoreScheme {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe slap a #[non_exhaustive] on this, so that new variants can be added after a potential 1.0 release

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in b9ca112

@alamb
Copy link
Contributor

alamb commented Jun 19, 2024

It appears this PR also has a clippy CI failure that we need to fix before merging

@orf
Copy link
Contributor Author

orf commented Jun 22, 2024

Hey, I will fix these issues up next week. Thanks for the review 😌

@drmorr0
Copy link

drmorr0 commented Jun 26, 2024

Would it be possible to add #[derive(Clone)] on this as well now that it's public?

edit: also not a maintainer

@alamb
Copy link
Contributor

alamb commented Jun 27, 2024

I took the liberty of adding additional docs / examples to this PR and merging up from main

@alamb
Copy link
Contributor

alamb commented Jun 28, 2024

I'll plan to merge this in the next few days unless there are any other comments

@alamb
Copy link
Contributor

alamb commented Jun 30, 2024

Thanks again @orf and @leoyvens and everyone else

@alamb alamb merged commit a4d2167 into apache:master Jun 30, 2024
13 checks passed
@orf orf deleted the obj-store-pub branch June 30, 2024 11:41
@orf
Copy link
Contributor Author

orf commented Jun 30, 2024

Thank you for merging this, and thank you for fixing it up for me! Sorry I didn’t manage to find the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ObjectStoreScheme in the object_store crate public
4 participants