refactor: parse table properties lazily - #3030
Conversation
04c0b44 to
ee0bac5
Compare
ee0bac5 to
9cb1bd3
Compare
|
I think this pr is ready for review, cc @kevinjqliu @CTTY @laskoviymishka |
laskoviymishka
left a comment
There was a problem hiding this comment.
the caching approach here is clean and the invariant is nicely enforced — properties stays pub(crate), only set_properties/remove_properties mutate it, and both re-parse, so the typed cache can't drift during update replay.
I'd hold this before merging though, for one thing.
Moving the property parse into the serde TryFrom paths means a metadata file that Java or PyIceberg reads fine — a commit.retry.num-retries typo, or a write.metadata.compression-codec value we don't recognize — now fails to deserialize entirely, so the whole table becomes unloadable rather than just that one property being wrong on access. The spec treats properties as an unvalidated map<string,string>, and today metadata_location.rs even swallows the codec parse error with unwrap_or(CompressionCodec::None). That's a real cross-engine interop regression. I'd keep the serde paths lenient (try_from(&properties).unwrap_or_default(), surfacing the error at access time instead), or write down that the strictness is intentional — left a note inline on the V3 path.
There's also a small cluster of consistency and test-coverage cleanups (an infallible -> Result left on metadata_compression_codec(), a Default impl that'd tidy several construction sites, some dropped compression-codec coverage) — all minor, none blocking, so I'll leave those to your judgment.
Once the leniency question is settled I'll take another pass.
| .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)?; |
There was a problem hiding this 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?
There was a problem hiding this comment.
Thanks for the review, I added a try_from_loosely method in #3044 so that we could parser properties in a reader friendly way, e.g. when one property failed to be parsed, we use its default value, rather than fail the whole parsing, wdyt?
3c2c805 to
122c552
Compare
122c552 to
dadf3ef
Compare
dadf3ef to
cb1d5d1
Compare
cb1d5d1 to
b93d99b
Compare
Which issue does this PR close?
What changes are included in this PR?
TablePropertieswith the lazyproperties_view!macro from feat: add lazy property view macro #3044.TableMetadata;table_properties()now returns a lightweight borrowed view.This gives metadata loading lenient raw-map semantics while keeping write and behavior settings strict at their point of use. It also avoids an eager parsed cache and its synchronization overhead.
Are these changes tested?
cargo test -p iceberg --lib(1,580 tests)cargo test -p iceberg-datafusion --lib(89 tests)cargo clippy -p iceberg-property-macro -p iceberg -p iceberg-datafusion --all-targets --all-features -- -D warningscargo public-api -p iceberg --all-features -ss | diff - crates/iceberg/public-api.txtcargo fmt --all -- --checkNew coverage verifies that V1, V2, and V3 metadata preserve invalid raw properties, unrelated getters remain usable, and only the invalid property getter fails.
AI Disclosure
This PR was developed with AI-assisted tooling.