laskoviymishka commented on code in PR #3030:
URL: https://github.com/apache/iceberg-rust/pull/3030#discussion_r3833951934
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1018,6 +1019,8 @@ pub(super) mod _serde {
})?
.into();
let default_partition_type =
default_spec.partition_type(current_schema)?;
+ let properties = value.properties.unwrap_or_default();
+ let table_properties = TableProperties::try_from(&properties)?;
Review Comment:
I think this is the one thing I'd want to settle before merge (same change
is in the V2 and V1 paths below). Parsing inside the serde `TryFrom` means a
metadata JSON that Java or PyIceberg reads fine — say
`commit.retry.num-retries=foo` from a human typo, or an unrecognized
`write.metadata.compression-codec` — now fails to deserialize at all. The whole
table becomes unloadable, not just that one property.
That's a behavior change from the raw-map approach, where the error only
surfaced when someone actually read the typed property, and
`metadata_location.rs` even swallowed the codec parse error with
`unwrap_or(CompressionCodec::None)`. The spec treats `properties` as an
unvalidated `map<string,string>`, and both Java (`PropertyUtil.propertyAsInt`
with a default) and PyIceberg validate lazily.
I'd keep the serde paths lenient — store the raw map and populate the cache
with `TableProperties::try_from(&properties).unwrap_or_default()` (needs a
`Default` impl), surfacing parse errors at access time or as a warning. If the
strictness is intentional, I'd want that written down, since it's a real
cross-engine interop regression. wdyt?
--
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]
