fix: support reading compressed metadata #1802
Conversation
colinmarc
force-pushed
the
metadata-compressed
branch
2 times, most recently
from
654de6b to
cd16381
Compare
| let metadata_content = input_file.read().await?; | ||
| let metadata = serde_json::from_slice::<TableMetadata>(&metadata_content)?; | ||
|
|
||
| let metadata = if metadata_location.as_ref().ends_with(".gz.metadata.json") { |
There was a problem hiding this comment.
Do we want to optionally support the Java Iceberg alternative?
The Java reference implementation can additionally read GZIP compressed files with the suffix metadata.json.gz.
There was a problem hiding this comment.
Seems better to have one convention, to me, but happy either way.
Even better would be peeking at the file and looking for the gzip magic number. If there's interest in that I can implement it. The wording of the spec ("some implementations require") seems to suggest it would be better to have no naming requirement at all.
There was a problem hiding this comment.
Even better would be peeking at the file and looking for the gzip magic number. If there's interest in that I can implement it.
That would be a really elegant solution, I think.
colinmarc
force-pushed
the
metadata-compressed
branch
2 times, most recently
from
9892bae to
011512a
Compare
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Minor performance nit.
|
|
||
| use _serde::TableMetadataEnum; | ||
| use chrono::{DateTime, Utc}; | ||
| use flate2::read::GzDecoder; |
There was a problem hiding this comment.
When you go to read metadata_content it's already in memory as a &[u8] so I think we should use flate2::bufread::GzDecoder here. It might be an imperceptible performance difference, but you never know how big metadata might get :)
There was a problem hiding this comment.
Hm, should be the opposite, no? With bufread we'll pay for an extra copy, but the "syscalls" (read) are free.
There was a problem hiding this comment.
Yeah you're right, I had it backwards in my head, sorry about that!
colinmarc
force-pushed
the
metadata-compressed
branch
2 times, most recently
from
2d87efe to
453dadc
Compare
|
Just found one case ( |
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @colinmarc for this pr!
| let metadata = if metadata_content.len() > 2 | ||
| && metadata_content[0] == 0x1F | ||
| && metadata_content[1] == 0x8B | ||
| { |
There was a problem hiding this comment.
Add a debug log here to explain why we choose to use try to decompress it?
There was a problem hiding this comment.
Would you like me to pull in a dependency? Neither tracing or log are available here.
There was a problem hiding this comment.
I think tracing is already there, anyway, I think the error message is good enough.
The spec mentions that metadata files "may be compressed with GZIP",
here:
https://iceberg.apache.org/spec/#table-metadata-and-snapshots
colinmarc
force-pushed
the
metadata-compressed
branch
from
453dadc to
dee387b
Compare
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @colinmarc for this fix!
3 participants
The spec mentions this naming convention here:
https://iceberg.apache.org/spec/#naming-for-gzip-compressed-metadata-json-files
Which issue does this PR close?
What changes are included in this PR?
Support for reading compressed metadata.
Are these changes tested?
Yes.