Skip to content

Add Attributes API (#5329)#5650

Merged
tustvold merged 3 commits into
apache:masterfrom
tustvold:add-attributes
Apr 16, 2024
Merged

Add Attributes API (#5329)#5650
tustvold merged 3 commits into
apache:masterfrom
tustvold:add-attributes

Conversation

@tustvold

@tustvold tustvold commented Apr 15, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes apache/arrow-rs-object-store#95
Closes apache/arrow-rs-object-store#95
Closes #4498

Rationale for this change

See tickets

This will provide the base that can later be extended to support object metadata apache/arrow-rs-object-store#145

What changes are included in this PR?

Are there any user-facing changes?

use std::ops::Deref;

/// Additional object attribute types
#[non_exhaustive]

Copy link
Copy Markdown
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


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
Copy Markdown
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:

  • Allow GCP to always specify a content type
  • Generalize to when we support further attributes that will likely diverge across the implementations (e.g. #4754)


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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@tustvold tustvold added the api-change Changes to the arrow API label Apr 17, 2024
alamb pushed a commit to alamb/arrow-rs that referenced this pull request Mar 20, 2025
* Add Attributes API (#5329)

* Clippy

* Emulator test tweaks
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

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