GH-50148: [C++] Add Content-Encoding support to S3 filesystem metadata - #50167
alytantawyy wants to merge 1 commit into
Conversation
This is weird, all extra jobs are running not only CUDA but R Extra, Packaging Extra, C++ Extra and it does not seem like any of the required labels were ever present. |
|
ok, the PR seems to be quite outdated with respect to main so the |
|
@alytantawyy Can you please first rebase or merge from git main as it seems this PR is based on a quite outdated snapshot of the source tree? |
25b01e0 to
4ddbbdb
Compare
|
Could you fix the lint failure? |
| static std::unordered_map<std::string, Setter> GetSetters() { | ||
| return {{"ACL", CannedACLSetter()}, | ||
| {"Cache-Control", StringSetter(&ObjectRequest::SetCacheControl)}, | ||
| {"Content-Encoding", ContentEncodingSetter()}, |
There was a problem hiding this comment.
Can we use StringSetter(&ObjectRequest::SetContentEncoding)?
| {"Content-Encoding", ContentEncodingSetter()}, | |
| {"Content-Encoding", StringSetter(&ObjectRequest::SetContentEncoding)}, |
Rationale for this change
The S3 filesystem metadata handling supported headers such as
Content-Type,Content-Language,Cache-Control, andExpires, but omittedContent-Encoding.As a result,
Content-Encodingwas not propagated when writing S3 objectmetadata, and it was also not returned when reading metadata back.
What changes are included in this PR?
Content-Encodingto S3 object metadata extractionContent-Encodingto the S3 metadata setter whitelistContent-EncodingContent-EncodingmetadataAre these changes tested?
Yes.