Add derive properties macro

[blackmwk]

Which issue does this PR close?

• Closes #.

What changes are included in this PR?

Are these changes tested?

AI Disclosure

[CTTY]

Thanks for the work!

[blackmwk]

I removed the property updating functionality since there is no clear requirements for now. cc @CTTY @kevinjqliu

[laskoviymishka]

Thanks for splitting the macro out of #2955. This is much closer to what I had in mind.

The two API issues I called out there are fixed too: there are no setters now, so the set_write_format_default(Puffin) vs from_properties mismatch is gone, and Copy fields return T instead of &T, which cleans up the call sites.

I’d still hold it for a few corners from the last review, plus one question about the split itself.

A few things still look open before this lands in public-api.txt:

• parse_with on Option<T> is a hard type error right now. The (Some(parse_with), _) arm fires for Option<T> too, so something returning Result<String, _> gets assigned to an Option<String> field. I’d either reject that combination or wrap the parsed value in Some(...).
• parse_properties_with without additional_keys silently changes from the documented 4-arg call to a 3-arg one. I’d make that an invalid combination instead.
• In the prefix path, if parsed.is_empty() { default } means a non-empty default disappears as soon as any matching key is present. I think the scan should just return the map it builds.
• The codegen expect() calls should probably become syn::Errors so bad macro input produces compile_error! rather than a proc-macro panic.
• The README examples use write.fanout.enabled and write.data.path, which aren’t real Iceberg properties. I’d use write.datafusion.fanout.enabled and write.metadata.path, and call out that default = true is engine-specific.
• A couple of trybuild compile-fail tests would be useful here since the diagnostics are part of how people will use the macro.

The bigger question is the split. This PR has the macro, but not the TableProperties port that would exercise it against the real property definitions. At the same time it is publish = true at 0.10.0 with nothing consuming it yet.

I was expecting the first split to be the macro plus the existing properties ported 1:1, with “no observable behavior changes” as the bar. That gives us a real consumer and proves the DSL works for the actual shape we care about. If you want to keep the macro standalone, I’d make it publish = false for now and link a follow-up issue for the port.

I’d still prefer to land the existing properties first, then add the new ones in smaller groups and check defaults against Java.

The macro itself looks close. The two things I’d want settled before approving are the parse_with / Option<T> case and whether this lands with a real consumer.

[laskoviymishka]

I don't think write.fanout.enabled is an actual Iceberg property — Java only has the (deprecated) write.spark.fanout.enabled, and in iceberg-rust the key is write.datafusion.fanout.enabled (PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED in crates/iceberg/src/spec/table_properties.rs). Same with write.data.path on line 66 — that's not a standard key either; we use write.metadata.path.

Since this README is teaching material in Iceberg's own repo, I'd use the real keys so nobody bakes a nonexistent one into a config. Worth a note that default = true for fanout matches our datafusion extension but is the opposite of the closest Java constant (SPARK_WRITE_PARTITIONED_FANOUT_ENABLED_DEFAULT = false), so flagging it as engine-specific would help.

[laskoviymishka]

This is the split I asked for on #2955 — thanks for pulling the macro out. It went a bit further than the 1:1 port though: the TableProperties port that would prove the DSL fits our real property structs isn't here yet, and the crate is publish = true at 0.10.0 with nothing consuming it. Validating the macro against the existing properties was the whole point of porting first.

I'd want the port alongside this — or publish = false plus a tracking issue until it lands, before the API locks into public-api.txt. #2955 linked #2877; this PR's body is the empty template, so it'd help to link an issue here too. wdyt?

[laskoviymishka]

pub(getter) borrows Rust's pub(crate)/pub(super) visibility syntax for something that isn't a visibility — it means "generate a getter". My worry is we've spoken for that syntax, so if we ever want real pub(crate) getters we've boxed ourselves in. Would a plain getter (or accessor) keyword read better? Non-blocking, but cheaper to settle before it lands in public-api.txt. wdyt?

[laskoviymishka]

from_properties returns Result<Self, String>, but the rest of the codebase speaks iceberg::Error/ErrorKind::DataInvalid — TableProperties::try_from and parse_property both return Result<_, iceberg::Error>. Every callsite in the eventual port would have to .map_err to bridge the two, which cuts against the "no observable behavior changes" bar.

I'd either generate an iceberg::Error (the macro itself needs no iceberg dep since the code expands in the consumer crate), make the error type configurable, or justify the String choice in the PR description.

[laskoviymishka]

These .expect()s run at macro-expansion time, so if an invariant ever slips they surface as error: proc-macro panicked with a backtrace rather than a clean compile_error!. The invariants are enforced by parse_property_field today, but that coupling is implicit and fragile.

I'd propagate a syn::Error via ok_or_else(|| Error::new_spanned(...)) instead — there are a few of these (also around lines 429, 459, and 499).

[laskoviymishka]

This if parsed.is_empty() { #default } means a non-empty default only applies when zero keys match — the moment one key matches, the default entries are dropped rather than merged. That's an asymmetric substitution that'll surprise anyone using a non-empty default map.

I'd drop the branch: a prefix scan should just produce its (possibly empty) map, and let an empty default fall out naturally from no matches. wdyt?

[laskoviymishka]

This is the same parse_with-on-Option<T> corner I flagged on #2955 — it's a hard type error now rather than a latent one. The (Some(parse_with), _) arm fires unconditionally, ignoring whether the field is Option<T>, so for an Option<String> field with a parse_with returning Result<String, _> the generated code assigns a String into an Option<String> — and the error points into macro output, with nothing rejecting it up front.

I'd either reject parse_with on Option<T>, or generate Some(#parse_with(value)?) for option fields.

[laskoviymishka]

You added the by-value Copy getters I asked for on #2955 (returning T rather than &T) — thanks, that drops the *properties.gc_enabled() awkwardness at call sites. One gap remains: is_copy_type detects Copy structurally from the AST, so it only recognizes the hardcoded primitive names — a user Copy newtype like Meters(u64) still falls back to the &T branch, which contradicts the README's promise that Copy types return T.

A proc-macro genuinely can't resolve Copy impls, so I'd at least document the limitation (by-value only for the listed primitives) and add a test or two so it can't silently drift. wdyt?

[laskoviymishka]

These runtime value-error cases are good, but the macro's compile-time error messages are just as much its public API and nothing covers them. I'd add a small tests/compile-fail/ with trybuild for the misuse paths — missing #[property], multiple of key/prefix/nested, nested + default, prefix on a non-HashMap, and the parse_properties_with/additional_keys mismatch from above — so the wording can't regress silently.

[laskoviymishka]

This only exercises the present-key path. When the key is absent, from_properties uses #default directly and skips parse_with entirely — so a default the parser would reject still slips through. I'd add a case that omits location and asserts the default, to lock in that the default bypasses the parser (or decide it shouldn't).