blackmwk commented on code in PR #2894:
URL: https://github.com/apache/iceberg-rust/pull/2894#discussion_r3948429401
##########
crates/iceberg/src/compression.rs:
##########
@@ -62,6 +66,22 @@ pub enum CompressionCodec {
Snappy,
}
+pub(crate) const TABLE_METADATA_SUPPORTED_COMPRESSION: &[CompressionCodec] = &[
+ CompressionCodec::None,
+ CompressionCodec::Gzip(GZIP_DEFAULT_LEVEL),
+ CompressionCodec::Zstd(ZSTD_DEFAULT_LEVEL),
+];
+
+pub(crate) const TABLE_METADATA_SUFFIX_TO_COMPRESSION: &[(&str,
CompressionCodec)] = &[
+ (ZSTD_SUFFIX, CompressionCodec::Zstd(ZSTD_DEFAULT_LEVEL)),
+ (GZIP_SUFFIX, CompressionCodec::Gzip(GZIP_DEFAULT_LEVEL)),
+];
+
+pub(crate) const TABLE_METADATA_MAGIC_TO_COMPRESSION: &[(&[u8],
CompressionCodec)] = &[
+ (GZIP_MAGIC, CompressionCodec::Gzip(GZIP_DEFAULT_LEVEL)),
+ (ZSTD_MAGIC, CompressionCodec::Zstd(ZSTD_DEFAULT_LEVEL)),
+];
Review Comment:
I think they should be const fn functions of CompressionCodec?
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -43,31 +69,14 @@ fn parse_metadata_compression(value: &str) ->
Result<CompressionCodec> {
let lowercase_value = value.to_lowercase();
// Use serde to parse the codec (which has rename_all = "lowercase")
- let codec: CompressionCodec =
serde_json::from_value(serde_json::Value::String(
- lowercase_value,
- ))
- .map_err(|_| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Invalid metadata compression codec: {value}. Only '{}' and
'{}' are supported.",
- CompressionCodec::None.name(),
- CompressionCodec::gzip_default().name()
- ),
- )
- })?;
-
- // Validate that only None and Gzip are used for metadata
- match codec {
- CompressionCodec::None | CompressionCodec::Gzip(_) => Ok(codec),
- _ => Err(Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Invalid metadata compression codec: {value}. Only '{}' and
'{}' are supported for metadata files.",
- CompressionCodec::None.name(),
- CompressionCodec::gzip_default().name()
- ),
- )),
+ let codec: CompressionCodec =
+ serde_json::from_value(serde_json::Value::String(lowercase_value))
+ .map_err(|_| invalid_metadata_compression_codec(value))?;
+
+ if TABLE_METADATA_SUPPORTED_COMPRESSION.contains(&codec) {
Review Comment:
This is obviously incorrect, you missed compression level, see
https://github.com/apache/iceberg-rust/pull/2894/changes#r3948429401
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
