Skip to content

feat: Derive TableProperties parsing - #2988

Merged
CTTY merged 3 commits into
mainfrom
ir-2877-table-properties-derive
Aug 19, 2026
Merged

feat: Derive TableProperties parsing#2988
CTTY merged 3 commits into
mainfrom
ir-2877-table-properties-derive

Conversation

@blackmwk

@blackmwk blackmwk commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

What changes are included in this PR?

  • Port the existing TableProperties keys, defaults, and custom parsers to #[derive(Properties)].
  • Preserve the existing TryFrom<&HashMap<String, String>> behavior and public fields so this layer remains source-compatible.
  • Generate read-only accessors and update the public API snapshot.
  • Add documentation to the macro-generated from_properties constructor.

This is the middle PR in stack #2990. It depends on #2987; #2989 migrates consumers and makes the backing fields private.

Are these changes tested?

  • cargo test -p iceberg spec::table_properties
  • Public API snapshot regenerated with cargo public-api -p iceberg --all-features -ss

AI Disclosure

This change was implemented and validated with OpenAI Codex assistance.

@blackmwk
blackmwk force-pushed the ir-2877-table-properties-derive branch 2 times, most recently from e8d8b3d to c4ee623 Compare August 12, 2026 03:37
Base automatically changed from ir-2877-pr2970-followups to main August 12, 2026 17:43
@blackmwk
blackmwk force-pushed the ir-2877-table-properties-derive branch from c4ee623 to 7753545 Compare August 12, 2026 17:44

@CTTY CTTY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! left one minor comment

Comment thread crates/iceberg/src/spec/table_properties.rs Outdated
@CTTY

CTTY commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

My AI flagged something interesting as well:

The blocking problem is packaging, not parsing. crates/iceberg is publish = true; crates/property-macro is publish = false and has never been on crates.io. Adding it as a regular dependency of iceberg breaks cargo publish --workspace, which is exactly what publish.yml runs on a release tag.

Comment thread crates/iceberg/src/spec/table_properties.rs
Port the existing TableProperties keys, defaults, and custom parsers to the Properties derive macro while preserving the TryFrom API and public fields for compatibility. Add generated getters and update the public API snapshot.\n\nCloses #2968.
@blackmwk
blackmwk force-pushed the ir-2877-table-properties-derive branch from 7753545 to f127686 Compare August 19, 2026 09:38
@blackmwk

Copy link
Copy Markdown
Contributor Author

My AI flagged something interesting as well:

The blocking problem is packaging, not parsing. crates/iceberg is publish = true; crates/property-macro is publish = false and has never been on crates.io. Adding it as a regular dependency of iceberg breaks cargo publish --workspace, which is exactly what publish.yml runs on a release tag.

Fixed.

@CTTY CTTY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@CTTY
CTTY merged commit 3c6c966 into main Aug 19, 2026
22 checks passed
@CTTY
CTTY deleted the ir-2877-table-properties-derive branch August 19, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A new TableProperties struct with the new derived property macro.

2 participants