Add an annotation framework for typed table properties

[blackmwk]

Which issue does this PR close?

• Closes [EPIC] Type safe TableProperties parsing. #2877.

What changes are included in this PR?

• Add the iceberg-property-macro crate with a Properties derive macro.
• Define table-property keys, defaults, documentation, and custom converters using field annotations.
• Generate Default, public getters, string-map parsing, and JSON serialization/deserialization.
• Keep TableProperties fields private.
• Change write.format.default from String to the typed DataFileFormat.
• Update existing consumers to use the generated getters.
• Update the public API snapshot.

Are these changes tested?

Yes.

• Added unit tests for default and overridden property values.
• Added JSON serialization and deserialization round-trip tests.
• Added coverage for optional properties and custom parsing behavior.
• Ran the Iceberg and DataFusion unit test suites.
• Ran Clippy with warnings denied.
• Verified formatting and the public API snapshot.

[blackmwk]

I think this pr is ready for review, cc @kevinjqliu @CTTY

TableProperties is now easier maintain, and we could expand it to typed catalog configs later.

[kevinjqliu]

This looks great. I iterated with codex on the PR review.

One thing that stood out to me for the new implementation is a lack of custom validators. For example, write.metadata.path previous implementation rejected empty values or trims trailing slashes.

I also feel like we might benefit from breaking this PR up into smaller chunks so we can more easily refactor the table properties in groups

[kevinjqliu]

is this behavior change intentional?

previous parser (parse_location_property) had custom logic to strip out the slashes.

And this test verifies that behavior

[kevinjqliu]

i think we have to publish the new crate, otherwise the iceberg crate cannot be packaged. 😭

cargo package -p iceberg --no-verify

[kevinjqliu]

This round trip changes a valid Parquet property into an invalid one:

write.parquet.compression-codec=uncompressed
→ CompressionCodec::None
→ write.parquet.compression-codec=none

Parquet expects uncompressed, not none.

Should serialization preserve the format-specific value?

[kevinjqliu]

nit this NameMapping change seems unrelated to the PR

[kevinjqliu]

self.table_properties()? parses every modeled property.
parse_metadata_file_compression only parsed what it needs.

An invalid unrelated value can therefore make this accessor fail. Parse only the required property here, or make typed access lazy.

[kevinjqliu]

write.delete.format.default should inherit write.format.default when absent, not always default to Parquet.

With only write.format.default=orc, the PR returns write_delete_format_default == Parquet

upstream documents the default as “data file format.”

[kevinjqliu]

Iceberg leaves these distribution properties unset so engines can choose their defaults.

Using DistributionMode::None conflates absence with an explicit "none" and imposes behavior that is not standardized. Consider Option<DistributionMode> to preserve that distinction.

[kevinjqliu]

The DataFileFormat enum is broader than this property’s valid domain.
For example, the parser currently accepts write.format.default=puffin, although only Parquet, Avro, and ORC are allowed.

The same applies to CompressionCodec, which permits invalid format/codec combinations. Consider property-specific validation.

[kevinjqliu]

This refactor changes existing behavior and drops several regression tests from table_properties.rs.

Behavior lost or changed:

• write.metadata.path no longer rejects empty values or trims trailing slashes.
• Every call to table_properties() eagerly parses the expanded set of modeled properties, so an invalid unrelated property can make an operation fail. Some previous paths, such as metadata compression, parsed only the required key.
• Metadata compression now accepts uncompressed; the previous allowlist was "", none, and gzip.

Missing regression coverage:

• Empty and trailing-slash metadata paths.
• Metadata-compression defaults, case-insensitive values, and invalid codecs.
• Invalid numeric and boolean values.
• CDC defaults, partial overrides, negative normalization levels, and parsing errors.

Could we preserve the previous behavior and port these regression tests as part of the refactor?

[laskoviymishka]

Kevin’s threads already cover the property-level behavior changes, so I mostly looked at the API introduced by this revision.

The main issue is that the generated setters don’t enforce the same validation as the parse path.

For example, set_write_format_default(DataFileFormat::Puffin) compiles, but parse_table_file_format rejects Puffin. So the setter can create a TableProperties value that from_properties would refuse to build, and that invalid value can then be serialized back into a property map.

Same problem with things like:

• set_write_parquet_compression_codec(CompressionCodec::Zlib)
• set_write_avro_compression_codec(CompressionCodec::Brotli)

Those are outside the allowed codec sets for those formats.

Now that the fields are private, setters are the main write API, so I think this should be resolved before public-api.txt locks in all 116 of them.

A few options seem reasonable:

• make setters fallible for fields using #[parse_with] / #[parse_properties_with], and reuse the same validator
• keep those setters pub(crate) and expose validated builder methods instead
• or, at minimum, document clearly that setters do not validate and callers are responsible for preserving invariants

A few other things I’d still look at:

• getters return &T even for Copy fields, so call sites end up doing things like *properties.gc_enabled(). Returning primitives by value in field_accessors would be nicer. Fine as a follow-up if you don’t want more churn here.
• the macro docs should spell out the four hook signatures. In particular, the parse hook gets the default by value while the write hook gets it by reference, and that isn’t obvious from the current docs.
• #[write_properties_with] without #[parse_properties_with] should probably fail at compile time. If that asymmetry is intentional, the docs should say what parse path is used instead.

I’m also still in favor of splitting the PR.

This revision makes the case pretty well: table_props.rs grew from 1129 to 1682 lines, while the macro crate itself stayed fairly straightforward. Most of the review effort is going into the property definitions and defaults, not the macro mechanism.

I’d be more comfortable landing the macro plus the existing properties as a 1:1 port first, with the bar being “no observable behavior changes,” then adding the new properties in smaller groups and checking their defaults against Java.

The macro itself looks close.

[laskoviymishka]

I'd make the setters for validated fields fallible, since as generated they bypass the validation the parse path enforces.

The setter is a bare assignment, so set_write_format_default(DataFileFormat::Puffin) compiles and leaves the struct in a state from_properties would have rejected — parse_table_file_format (table_props.rs:188) explicitly errors on Puffin. Same shape for set_write_parquet_compression_codec(CompressionCodec::Zlib) and set_write_avro_compression_codec(CompressionCodec::Brotli), neither of which is in the respective allowlist. So the value round-trips out to a property map that this crate itself won't read back.

For fields carrying a #[parse_with] / #[parse_properties_with] validator I'd have the setter call the same validator and return Result<()>. If keeping them infallible matters more, then narrowing them to pub(crate) and exposing validated builder methods would work too — or at minimum a doc line saying the setter is unvalidated so callers know the invariant is theirs to hold. wdyt?

[laskoviymishka]

I'd put the actual hook signatures in this doc block. It says write hooks are passed the field default and that additional_key is passed after the primary key, but never the shapes, so implementing a hook for a new field means reading parse_field and write_field to recover them.

From the generated call sites at :481 and :652:

// parse_properties_with, with additional_key
fn(&HashMap<String, String>, key: &str, additional_key: &str, default: T) -> Result<T, impl Display>
// without additional_key: fn(&HashMap<String, String>, key: &str, default: T) -> Result<T, impl Display>

// write_properties_with, with additional_key
fn(&T, &mut HashMap<String, String>, key: &str, additional_key: &str, default: &T)
// without additional_key: fn(&T, &mut HashMap<String, String>, key: &str, default: &T)

Two of these aren't guessable: default arrives by value in the parse hook but by reference in the write hook, and parse_with always receives &str even when the field is Option<T> — whereas serialize_with on an Option<T> field receives &Option<T> (see the other thread on :675). Getting any of them wrong surfaces as E0308 at the #[derive(Properties)] line with no pointer back to the attribute, so a few /// lines here would save a round trip through the macro source. A doctest exercising one hook would be even better, since it can't drift.

[laskoviymishka]

I'd return T rather than &T for the Copy fields here. The template is pub fn #ident(&self) -> &#ty for every field, so gc_enabled() hands back &bool and commit_retry_num_retries() hands back &usize.

The cost shows up at every call site in this PR: *properties.gc_enabled() in catalog/utils.rs, *props.write_target_file_size_bytes() in parquet_writer.rs, and the same in transaction/mod.rs and expire_snapshots.rs. None of these fields carry a lifetime and the struct is Clone, so the reference buys nothing.

Gating on the primitive types in field_accessors — bool, the integer types, f64 — and emitting -> #ty / self.#ident for those would drop the derefs while leaving String, Option<T> and HashMap getters as they are. Happy to see this as a follow-up if you'd rather not churn the call sites in this PR, but it's cheaper to settle before the getter names land in public-api.txt.

[laskoviymishka]

This gate catches #[additional_key] with neither hook, but not either hook without the other, so both asymmetric combinations compile silently.

#[write_properties_with(f)] on its own serializes through f and parses through the standard FromStr branch at :490, so the read and write paths end up enforcing different invariants — for exactly the multi-key fields these hooks exist to handle. The reverse pairing writes via ToString and loses whichever key the hook meant to set. Every use in table_props.rs today pairs them, so nothing is broken yet.

I'd reject an unpaired annotation here. If the asymmetric case is meant to be allowed, a note in the doc block naming which path the unhooked side falls back to would cover it.

[laskoviymishka]

For an Option<T> field serialize_with gets &Option<T>, while the bare-T branch at :687 gets &T — and the is_some() guard at :681 means the hook can only ever be called with Some(_).

That asymmetry is already costing something concrete: serialize_name_mapping (table_props.rs:404) has to take &Option<NameMapping> and carry .expect("checked is_some before serialization") for a case the generated code has ruled out. The signature encodes an invariant the type system isn't being told about.

Unwrapping before the call would make both branches hand over &T and let that expect go away:

Some(serialize_with) => quote!(#serialize_with(
    self.#ident.as_ref().expect("checked is_some above")
)),

One hook uses this today, so it's about as cheap to change as it will ever be. Documenting the asymmetry instead would also be fine, as long as the next person writing an Option hook finds out before the compiler tells them at the derive site.

[blackmwk]

Hi, @laskoviymishka I have create a separate pr for the framework, see #2970