You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "criccomini (via GitHub)" <gi...@apache.org> on 2024/06/18 20:09:29 UTC
[PR] Add user defined metadata [arrow-rs]
criccomini opened a new pull request, #5915:
URL: https://github.com/apache/arrow-rs/pull/5915
# Which issue does this PR close?
Closes #4754
# Rationale for this change
We would like to be able to read and write user-defined metadata for AWS, GCP, and Azure. This use-case is mentioned briefly in #4754 before the conversation shifts to tags. It appears that tags were implemented, but user-defined metadata has not yet been.
# What changes are included in this PR?
- Add user-defined metadata for AWS, GCP, and Azure clients.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
I've added a `Metadata(String)` attribute to the `Attribute` enum. Other than that I don't believe there are any user-facing changes.
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
I do not believe there are any breaking changes. The user-facing change (`Attributes::Metadata(String)`) is additive and should be compatible. The one caveat here is I'm not sure how Rust handles enums (open vs. closed). If it handles them the way Java does (i.e. adding new enums can break old code) then this would be a breaking change. I'll leave it to the Rust experts to clarify this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#discussion_r1645291141
##########
object_store/src/client/get.rs:
##########
@@ -221,6 +221,23 @@ fn get_result<T: GetClient>(
)
);
+ // Add attributes that match the user-defined metadata prefix (e.g. x-amz-meta-)
+ let attributes = if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix {
+ let mut attributes = attributes.clone();
Review Comment:
It's better to not clone entire `attributes`. How about making the previous one `mut` and modify it in place?
For example:
```rust
if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix {
...
attributes.insert(key, val);
}
```
##########
object_store/src/aws/client.rs:
##########
@@ -61,6 +61,7 @@ use std::sync::Arc;
const VERSION_HEADER: &str = "x-amz-version-id";
const SHA256_CHECKSUM: &str = "x-amz-checksum-sha256";
+const USER_DEFINED_METADATA_HEADER_PREFIX: &str = "x-amz-meta-";
Review Comment:
nit: maybe `METADATA_HEADER_PREFIX` is clear enough?
##########
object_store/src/aws/client.rs:
##########
@@ -642,6 +647,7 @@ impl GetClient for S3Client {
etag_required: false,
last_modified_required: false,
version_header: Some(VERSION_HEADER),
+ user_defined_metadata_prefix: Some(USER_DEFINED_METADATA_HEADER_PREFIX),
Review Comment:
nit: how about using `metadata_header_prefix`?
##########
object_store/src/http/client.rs:
##########
@@ -183,6 +183,8 @@ impl Client {
has_content_type = true;
builder.header(CONTENT_TYPE, v.as_ref())
}
+ // Ignore metadata attributes
+ Attribute::Metadata(_) => builder,
Review Comment:
WebDAV handles custom metadata in `prop`, which differs from other object storage services. We can create an issue to track this.
##########
object_store/src/aws/client.rs:
##########
@@ -326,6 +327,10 @@ impl<'a> Request<'a> {
has_content_type = true;
builder.header(CONTENT_TYPE, v.as_ref())
}
+ Attribute::Metadata(k_suffix) => builder.header(
+ &format!("{}{}", USER_DEFINED_METADATA_HEADER_PREFIX, k_suffix),
Review Comment:
Maybe we should use `self.config.metadata_header_prefix`?
##########
object_store/src/client/get.rs:
##########
@@ -221,6 +221,23 @@ fn get_result<T: GetClient>(
)
);
+ // Add attributes that match the user-defined metadata prefix (e.g. x-amz-meta-)
+ let attributes = if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix {
+ let mut attributes = attributes.clone();
+ for (key, val) in response.headers() {
Review Comment:
We can use `strip_prefix` to make logic here more clear:
```rust
let mut attributes = attributes;
for (key, val) in response.headers() {
if let Some(suffix) = key.as_str().strip_prefix(prefix) {
if let Ok(val_str) = val.to_str() {
attributes.insert(
Attribute::Metadata(suffix.to_string()),
val_str.to_string().into(),
);
}
}
}
attributes
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#discussion_r1650034421
##########
object_store/src/client/get.rs:
##########
@@ -221,6 +221,20 @@ fn get_result<T: GetClient>(
)
);
+ // Add attributes that match the user-defined metadata prefix (e.g. x-amz-meta-)
+ if let Some(prefix) = T::HEADER_CONFIG.user_defined_metadata_prefix {
+ for (key, val) in response.headers() {
+ if let Some(suffix) = key.as_str().strip_prefix(prefix) {
+ if let Ok(val_str) = val.to_str() {
Review Comment:
I wonder if we should error here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#discussion_r1650034682
##########
object_store/src/http/client.rs:
##########
@@ -183,6 +183,8 @@ impl Client {
has_content_type = true;
builder.header(CONTENT_TYPE, v.as_ref())
}
+ // Ignore metadata attributes
+ Attribute::Metadata(_) => builder,
Review Comment:
In the meantime I think we should return an error as per https://docs.rs/object_store/latest/object_store/struct.PutOptions.html#structfield.attributes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "criccomini (via GitHub)" <gi...@apache.org>.
criccomini commented on PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#issuecomment-2176889994
There are a few implementation details I'd like to call out:
1. The `reqwest` header name handling returns all custom headers as lower case. So if a user sets a `Metadata("FOO")`, the results returned in GetResult will have `Metadata("foo")`. This is somewhat unintuitive.
2. I elected to make `USER_DEFINED_METADATA_HEADER_PREFIX` a const rather than static because it's referenced in certain `const` spots. Rust didn't like `static` variables referenced from `const`.
3. I'm not thrilled about the `get.rs` code I added, but I couldn't figure out a better way. We need to iterate over all headers with the user-defined prefix (cloud-specific) and add them to `attributes`. I'm open to alternative approaches.
4. I did not add WebDav (http) support. The client simply ignores `Metadata` attributes on `put()` right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#issuecomment-2177407329
> The one caveat here is I'm not sure how Rust handles enums (open vs. closed).
Rust handles this by [non_exhaustive](https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute) attribute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add user defined metadata [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5915:
URL: https://github.com/apache/arrow-rs/pull/5915#discussion_r1650034289
##########
object_store/src/attributes.rs:
##########
@@ -45,6 +45,10 @@ pub enum Attribute {
///
/// See [Cache-Control](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control)
CacheControl,
+ /// Specifies a user-defined metadata field for the object
+ ///
+ /// The String is a user-defined key
+ Metadata(String),
Review Comment:
It's likely a silliness that will make no measurable performance difference, but given these will often be static strings, I wonder what you think of making this `Cow<'static, str>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org