LuciferYang commented on issue #2028: URL: https://github.com/apache/iceberg-rust/issues/2028#issuecomment-5023023703
Hi @liurenjie1024, I'd like to take this one. Before I open a PR, here's my read of the two earlier attempts so we're on the same page. #2037 added typed fields but kept the unknown keys in a separate `additional_properties` map next to them. Two problems came up: it cloned the whole map into that side field, so already-parsed keys were stored twice, and its `new()` used `unwrap_or_default()`, which quietly dropped parse errors instead of surfacing them. You also asked there for the builder to stay on `HashMap<String, String>` with an extra field, for ser/de to live in `TableMetadata`'s own path rather than on `TableProperties`, and for parsing to go through `TryFrom` instead of `unwrap`. #2469 went a different way: `TableProperties` owned the raw `HashMap` itself and exposed typed getters plus `get`/`contains_key`/`iter`. @dannycjones reviewed it in detail (return `&str` rather than `&String`, add an `into_inner` so serialization doesn't clone, don't expose the `HashMap` directly), but it stalled without a follow-up and was never approved. My plan is to build on the #2469 approach and fold in the guidance you left on #2037. Two things I want to settle before writing code. The first is when validation should run. Today it's lazy: a bad value like a non-numeric `commit.retry.num-retries` only errors when someone calls `table_properties()` (table_metadata.rs:380). Once `properties` becomes a typed field, that parse moves into `TableMetadata`'s deserialize path, so the same bad value would fail loading the entire table instead. Your "use `TryFrom`, don't `unwrap`" note reads like you want errors to propagate, so I'd lean that way, but I want to confirm you're fine with a load-time failure rather than keeping load lenient and defaulting unparseable known keys. The second is the builder. Following your earlier note, I'll keep `TableMetadataBuilder` on `HashMap<String, String>` and only turn it into `TableProperties` at `build()`. Let me know if that's still what you want. On scope: `properties()` would return `&TableProperties` instead of `&HashMap`, which is a public API break, so I'll regenerate the baselines with `make generate-public-api`. I'll drop the now-redundant `table_properties()` and update its callers in `transaction/mod.rs`, `transaction/expire_snapshots.rs`, `encryption/manager.rs`, `catalog/utils.rs`, and the DataFusion write path. Around 15 files, no new dependencies. -- 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]
