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

Add Attributes API (#5329) #5650

Merged
merged 3 commits into from
Apr 16, 2024
Merged

Add Attributes API (#5329) #5650

merged 3 commits into from
Apr 16, 2024

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Apr 15, 2024

Which issue does this PR close?

Closes #5329
Closes #5329
Closes #4498

Rationale for this change

See tickets

This will provide the base that can later be extended to support object metadata #4754

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the object-store Object Store Interface label Apr 15, 2024
use std::ops::Deref;

/// Additional object attribute types
#[non_exhaustive]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should allow us to add new attributes without it being a breaking change

@@ -363,8 +362,21 @@ impl S3Client {
)
}

if let Some(value) = self.config.client_options.get_content_type(path) {
builder = builder.header(CONTENT_TYPE, value);
let mut has_content_type = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried very hard to avoid this getting duplicated in the various implementations, however, it became very hard to devise something that would:


/// Additional object attribute types
#[non_exhaustive]
#[derive(Debug, Hash, Eq, PartialEq, Clone)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This explicitly isn't Copy to support user defined metadata in future, which will need to have a Metadata(AttributeValue) variant or similar


/// The value of an [`Attribute`]
///
/// Provides efficient conversion from both static and owned strings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given how frequently the values will be static strings, I felt it worth the complexity to optimise explicitly for this

/// Unlike [`ObjectMeta`](crate::ObjectMeta), [`Attributes`] are not returned by
/// listing APIs
#[derive(Debug, Default, Eq, PartialEq, Clone)]
pub struct Attributes(HashMap<Attribute, AttributeValue>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated making this an Arc wrapped data structure and then having an AttributesMut version, however, I decided this wasn't really worth it given that most use-cases will be constructing this per-request anyway (that is kind of the point).

@tustvold
Copy link
Contributor Author

Interestingly the emulators seem to not like this, I will investigate further tomorrow

@tustvold tustvold merged commit f276528 into apache:master Apr 16, 2024
13 checks passed
@tustvold tustvold added the api-change Changes to the arrow API label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

object_store: allow setting content-type per request object_store: Cache-Control header usage
2 participants