feat: add lazy property view macro by blackmwk · Pull Request #3044 · apache/iceberg-rust

blackmwk · 2026-08-22T09:19:47Z

Which issue does this PR close?

Related to [EPIC] Type safe TableProperties parsing. #2877.

What changes are included in this PR?

Add a function-like properties_view! macro that defines a lightweight view over HashMap<String, String>.
Parse only the requested field when its generated getter is called; construction performs no parsing.
Return Result<T> from each getter, use defaults only for absent keys, and preserve errors for present invalid values.
Support the same exact-key, optional, prefix, nested, and custom-parser property declarations as Properties.
Keep the existing owned Properties derive and strict from_properties behavior unchanged.
Document the generated API, borrowing model, and repeated-getter behavior.

Are these changes tested?

cargo test -p iceberg-property-macro
cargo clippy -p iceberg-property-macro --all-targets -- -D warnings

The tests cover lazy independent parsing, defaults, invalid values, custom parsers, prefix fields, nested views, and pointer-sized view storage.

AI Disclosure

This PR was developed with AI-assisted tooling.

blackmwk · 2026-08-25T02:19:48Z

Hi, @laskoviymishka @CTTY this pr is ready for reivew, PTAL

CTTY

The change looks good to me, but I don't quite understand the intention here: why do we want an infallible properties parser?

I think the value should only fall back to default when it's unset. Invalid properties should be rejected

CTTY · 2026-08-25T21:26:29Z

+                    ::std::string::String,
+                    ::std::string::String,
+                >,
+            ) -> ::iceberg::Result<Self> {


This seems infallible

That's a good point, but I'm hesitating to the method signature. I prefer to keep the return type falliable so that in future the signature will not be broken when introducing other changes.

blackmwk · 2026-08-26T00:34:15Z

The change looks good to me, but I don't quite understand the intention here: why do we want an infallible properties parser?

It's used to address this comment: #3030 (comment)

I think the value should only fall back to default when it's unset. Invalid properties should be rejected

The motivation is that we should be less strict when reading, but more strict when writing, according to some discussion earlier in dev list. Also this is to be align with java/python libraries behavior.

laskoviymishka

The derive-macro approach here is clean — generating the loose constructor from the same field definitions as from_properties keeps the two in lockstep, and the README additions read well.

I'd hold this before merging though, mostly around one design question. @CTTY already flagged the core of it and I'm with them:

I think the value should only fall back to default when it's unset. Invalid properties should be rejected

The angle I'd add on top is correctness. try_from_loosely treats "key absent" and "key present but unparseable" identically — both silently become the default. For advisory hints that's fine, but TableProperties also drives write-path settings: commit.retry.*, the write.*.path locations, the metadata compression codec. Silently defaulting one of those means a table configured with write.metadata.compression-codec=zstd writes uncompressed metadata, or a bad write.data.path lands data at the table root — invisibly, with no error and no log. Since from_properties already defaults absent keys, the only new behavior this adds is swallowing parse errors on values that are actually set, which is exactly the case I'd argue should stay an error. That's the piece I'd want intent on, especially as the reply on CTTY's thread came through empty.

A few things I'd want settled before merge:

the intent question on CTTY's thread — what use case needs tolerating present-but-invalid values rather than just absent ones?
if lenient parsing is genuinely wanted, distinguish advisory props (safe to default) from write-path props (location / codec / retry) that shouldn't be, and at minimum log discarded values instead of dropping them silently
the prefix-map path currently discards all valid sibling entries when any single one fails (noted inline) — that one looks like a straight bug regardless of how the design lands

Once the intent's spelled out and that thread's resolved, happy to take another pass and approve.

laskoviymishka · 2026-08-26T12:00:38Z

 pub fn iceberg::spec::TableProperties::parquet_page_row_limit(&self) -> usize
 pub fn iceberg::spec::TableProperties::parquet_page_size_bytes(&self) -> usize
 pub fn iceberg::spec::TableProperties::parquet_row_group_size_bytes(&self) -> usize
+pub fn iceberg::spec::TableProperties::try_from_loosely(properties: &std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>) -> iceberg::Result<Self>


exposing this at iceberg::spec::TableProperties makes it the "easy" path for anyone who just wants to dodge a parse error — and TableProperties is exactly the struct that drives Parquet params, commit retry, and write locations. A caller reaching for it to avoid a crash silently inherits wrong write settings.

If we keep a loose variant, I'd lean toward exposing it only on structs that opt in, or splitting advisory props (safe to default) from write-path ones, rather than blanket-applying it to the primary write-config struct. This ties into CTTY's thread — wdyt?

laskoviymishka · 2026-08-26T12:00:38Z

+
+            /// Parses this typed property set from a flat string-to-string map,
+            /// using each field's default when that field cannot be parsed.
+            pub fn try_from_loosely(


every arm in the loose path is infallible (unwrap_or_else / unwrap_or_default, and the nested try_from_loosely is itself always-Ok), so this returns Result<Self> but can never return Err. The try_ prefix and the doc's "when that field cannot be parsed" both imply a fallibility that isn't there — callers will .unwrap() on something that can't panic.

I'd either return Self and rename it (from_properties_lossy?), or keep Result<Self> for future-proofing but document that it always succeeds today.

laskoviymishka · 2026-08-26T12:00:38Z

+                    &[#(#additional_keys),*],
+                    #default,
+                )
+                .unwrap_or_else(|_| #default)


#default lands here twice — once passed into parse_properties_with as the 4th arg, once in the unwrap_or_else closure — so on the error path it's evaluated twice. Harmless for a literal, but a default expressed as a function call would run twice with no signal.

I'd hoist it into a let binding above the call and reuse it.

laskoviymishka · 2026-08-26T12:00:38Z

+                .collect::<::iceberg::Result<::std::collections::HashMap<_, _>>>()
+        };
+        let parse = if loosely {
+            quote!(#parse.unwrap_or_default())


this one looks like a straight bug independent of the design question. collect() into a single Result short-circuits on the first Err, and unwrap_or_default() then throws away the whole map — not just the offending entry. One malformed key under a shared prefix (per-column bloom-filter FPP, say) silently zeroes every valid sibling under that prefix.

I'd skip only the failing entry in loose mode rather than poisoning the collection — move the .ok() inside the filter_map so a bad value drops just its own key. Nothing currently tests this with more than one entry, so the regression is invisible (see my note on the test).

laskoviymishka · 2026-08-26T12:00:38Z

-                )
-            })?
-        },
+    let parse = if loosely {


the loosely flag forks this whole six-arm match into two near-identical copies that differ only in the error-handling tail — and the same split shows up in the nested / parse_properties_with / prefix branches above. Any new field category has to be updated in both halves, and forgetting one diverges strict vs loose with no compile error.

Could we extract the raw parse expression per arm once, then apply a strict-wrapper vs loose-wrapper over it? Would roughly halve this function. wdyt?

laskoviymishka · 2026-08-26T12:00:38Z

+        match (&field.parse_with, &field.option_inner_type) {
+            (Some(parse_with), Some(inner_type)) => quote! {
+                #parse_with(value)
+                    .map(|parsed: #inner_type| Some(parsed))


.map(|parsed: #inner_type| Some(parsed)) trips clippy::redundant_closure_for_method_calls, and since this expands into downstream crates, any consumer running cargo clippy -- -D warnings on an Option<T> field with parse_with would fail to build. The sibling arms already use .map(Some) — I'd match them here:

#parse_with(value).map(Some).unwrap_or_else(|_| #default)

laskoviymishka · 2026-08-26T12:00:38Z

+                    .unwrap_or_else(|_| #default)
+            },
+            (None, None) => quote! {
+                value.parse::<#ty>().unwrap_or_else(|_| #default)


this is CTTY's concern made concrete: a present-but-invalid value silently becomes the default here. commit.retry.num-retries=abc fails to parse and the table quietly runs with 4 retries — the user set the key, believes it's active, and gets no error or log.

Worth noting from_properties already returns the default when the key is absent; the only new behavior this adds is swallowing parse errors on values that are set — which is exactly the case I'd argue should stay an error. Happy to be convinced there's a real use case, but I'd want it spelled out on CTTY's thread first rather than replied here.

laskoviymishka · 2026-08-26T12:00:38Z

 }

+#[test]
+fn loose_parsing_defaults_only_invalid_fields() {


every new loose test asserts invalid→default, but none asserts that valid input actually parses through the loose path. If a bug made try_from_loosely always return defaults, all of these would still pass.

I'd add a case with all-valid values (RETRIES=8 → retries()==8, a location that trims to a real path) so we're pinning both directions.

laskoviymishka · 2026-08-26T12:00:38Z

+    assert_eq!(properties.owner().as_deref(), Some("iceberg"));
+    assert_eq!(properties.format(), "orc");
+    assert!(properties.fanout_enabled());
+    assert!(properties.column_fpp().is_empty());


with a single prefix entry this assertion can't tell "discard everything on first error" apart from "skip only the bad entry" — both leave column_fpp empty, so the all-or-nothing bug I flagged on properties.rs:494 slips right through here. I'd add a second, valid COLUMN_FPP entry (e.g. 0.01) and assert it survives alongside the bad one.

blackmwk changed the title ~~ir 2877 try from loosely~~ feat: add loose property parsing to Properties derive Aug 22, 2026

blackmwk requested review from CTTY, kevinjqliu and laskoviymishka August 22, 2026 09:26

blackmwk mentioned this pull request Aug 22, 2026

refactor: parse table properties lazily #3030

Open

CTTY reviewed Aug 25, 2026

View reviewed changes

laskoviymishka requested changes Aug 26, 2026

View reviewed changes

blackmwk force-pushed the ir-2877-try-from-loosely branch from 63b64a0 to 0dae89d Compare August 27, 2026 08:21

blackmwk changed the title ~~feat: add loose property parsing to Properties derive~~ feat: add lazy property view macro Aug 27, 2026

blackmwk force-pushed the ir-2877-try-from-loosely branch from 0dae89d to c94e69a Compare August 27, 2026 08:47

feat: add lazy property view macro

964c01d

blackmwk force-pushed the ir-2877-try-from-loosely branch from c94e69a to 964c01d Compare August 27, 2026 09:00

Uh oh!

Conversation

blackmwk commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

blackmwk commented Aug 25, 2026

Uh oh!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackmwk commented Aug 26, 2026

Uh oh!

laskoviymishka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blackmwk commented Aug 22, 2026 •

edited

Loading

laskoviymishka left a comment •

edited

Loading