Add an internal Properties derive macro by blackmwk · Pull Request #2970 · apache/iceberg-rust

blackmwk · 2026-08-06T06:24:38Z

Which issue does this PR close?

Closes A property derive macro. #2967.

What changes are included in this PR?

Add an internal iceberg-property-macro crate with a Properties derive macro for parsing typed structs from HashMap<String, String>.
Generate a public from_properties constructor returning iceberg::Result<Self> and opt-in read-only getters without implicitly deriving Default, Serialize, Deserialize, or other traits.
Report generated parsing failures as iceberg::Error with ErrorKind::DataInvalid and property-key context.
Support exact keys, defaults, optional values, case-insensitive booleans, prefixed maps, nested property structs, custom value parsers, and custom full-map parsers with additional keys.
Validate invalid attribute combinations during macro expansion and emit compile_error! diagnostics instead of proc-macro panics.
Document the complete API, including ordinary Rust derives, optional custom parsing, prefix behavior, and the structural limitation of by-value Copy getters.
Keep the crate unpublished until a production consumer such as TableProperties validates the final public API.

Are these changes tested?

Yes.

Unit tests cover defaults, overrides, optional and nested values, prefix maps, custom parsers, Iceberg error kinds and context, ordinary derives, and getter return behavior.
Trybuild compile-fail tests cover missing and contradictory annotations, invalid nested and prefix declarations, and both invalid parse_properties_with/additional_keys combinations.
README examples run as doctests.
Verified with nightly rustfmt and Clippy with warnings denied.
Verified compilation with the workspace MSRV, Rust 1.94.

AI Disclosure

This PR was developed with assistance from OpenAI Codex. The generated changes were reviewed and validated with the tests and checks listed above.

CTTY

Thanks for the work!

blackmwk · 2026-08-07T09:15:46Z

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 · 2026-08-07T22:24:15Z

+
+const RETRIES: &str = "commit.retry.num-retries";
+const OWNER: &str = "owner";
+const FANOUT: &str = "write.fanout.enabled";


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.

write.data.path is in the spec, so it's correct. write.fanout.enabled is not an actual Iceberg property (seems an engine specific property as you observed).

laskoviymishka · 2026-08-07T22:24:15Z

+edition = { workspace = true }
+homepage = { workspace = true }
+name = "iceberg-property-macro"
+publish = true


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?

Fixed, I agree that we should mark it as non publish for now.

laskoviymishka · 2026-08-07T22:24:15Z

+
+impl Parse for PublicGetter {
+    fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
+        input.parse::<Token![pub]>()?;


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 · 2026-08-07T22:24:15Z

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


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 · 2026-08-07T22:24:15Z

+    let default = typed_default(field);
+
+    if let Some(parse_properties_with) = &field.parse_properties_with {
+        let key = field.key.as_ref().expect("exact-key fields have a key");


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 · 2026-08-07T22:24:15Z

+                        ::std::collections::HashMap<_, _>,
+                        ::std::string::String,
+                    >>()?;
+                if parsed.is_empty() {


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 · 2026-08-07T22:24:16Z

+
+    let key = field.key.as_ref().expect("exact-key fields have a key");
+    let parse = match (&field.parse_with, &field.option_inner_type) {
+        (Some(parse_with), _) => quote! {


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 · 2026-08-07T22:24:16Z

+    is_named_type(ty, "bool")
+}
+
+fn is_copy_type(ty: &Type) -> bool {


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 · 2026-08-07T22:24:16Z

+
+#[test]
+fn reports_the_property_with_an_invalid_value() {
+    let numeric_error = TestProperties::from_properties(&HashMap::from([(


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 · 2026-08-07T22:24:16Z

+
+#[test]
+fn custom_single_value_parser_can_validate_and_normalize() {
+    let parsed = ValidatedProperties::from_properties(&HashMap::from([(


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).

Fixed with a default case. I'm hesitating to add a check for default value using parse method. I'm not sure using parser method to validate default value is the right direction to go. Sometimes we may don't need default value to follow rules from user passed value.

anoopj · 2026-08-09T23:57:56Z

  "crates/catalog/*",
  "crates/examples",
  "crates/iceberg",
+  "crates/property-macro",


Nit: sort alphabetically?

blackmwk · 2026-08-10T12:06:48Z

Comments resolved, cc @laskoviymishka @CTTY PTAL

blackmwk added 2 commits August 6, 2026 14:24

Add derive properties macro

894637b

Ready

a1fd056

blackmwk requested review from CTTY, Xuanwo and kevinjqliu August 6, 2026 07:39

blackmwk mentioned this pull request Aug 6, 2026

[EPIC] Type safe TableProperties parsing. #2877

Open

Fix property macro CI checks

0068cce

CTTY reviewed Aug 6, 2026

View reviewed changes

Comment thread crates/property-macro/README.md Outdated

Comment thread crates/property-macro/tests/properties.rs Outdated

blackmwk added 3 commits August 7, 2026 16:57

Remove write support

4e35b2c

Force attributes

5896eea

Merge remote-tracking branch 'upstream/main' into ir-2967

e9b6b6f

blackmwk mentioned this pull request Aug 7, 2026

Add an annotation framework for typed table properties #2955

Draft

blackmwk requested a review from laskoviymishka August 7, 2026 14:33

laskoviymishka reviewed Aug 7, 2026

View reviewed changes

anoopj reviewed Aug 9, 2026

View reviewed changes

blackmwk added 2 commits August 10, 2026 14:58

Address property macro review feedback

4f9f7da

Merge remote-tracking branch 'upstream/main' into ir-2967

49af143

blackmwk changed the title ~~Add derive properties macro~~ Add an internal Properties derive macro Aug 10, 2026

blackmwk and others added 5 commits August 10, 2026 15:40

Use Iceberg errors in generated property API

ccee9f8

Require Iceberg results from property parsers

6ea3bb5

Manage property macro dependencies in workspace

993188f

Sort workspace members alphabetically

1faa7fc

Exclude trybuild snapshots from license checks

7435f86

Uh oh!

Conversation

blackmwk commented Aug 6, 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!

CTTY left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

blackmwk commented Aug 7, 2026

Uh oh!

laskoviymishka 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!

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!

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!

blackmwk commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blackmwk commented Aug 6, 2026 •

edited

Loading