refactor: replace HashMap with TableProperties in TableMetadata (#2028)

[vigneshsiva11]

Which issue does this PR close?

Closes #2028

What changes are included in this PR?

This PR refactors TableMetadata to use the typed TableProperties struct instead of a raw HashMap<String, String>, addressing issue #2028. This improvement enhances type safety and reduces repeated allocations by returning references to structured data rather than reconstructing property maps on every call.

Key Modifications:

• TableMetadata

  • Updated the properties field type
  • Simplified the table_properties() getter to return a reference to the struct

• TableProperties

  • Added Default, Clone, PartialEq, and Eq trait implementations
  • Added an other: HashMap<String, String> field to maintain compatibility with custom or non-explicitly mapped properties
  • Implemented a new() constructor to facilitate conversion from raw maps

• TableMetadataBuilder

  • Updated the builder logic to initialize TableMetadata using the new TableProperties type

• Documentation

  • Added required doc comments to new fields and methods to satisfy the project's strict linting rules (missing_docs)

Are these changes tested?

Yes.

• Verified using cargo check -p iceberg with no errors or lint warnings
• Code formatted using cargo fmt --all

The logic ensures existing property-based workflows remain functional through the internal other property map delegation.

[Copilot]

Pull request overview

This PR refactors TableMetadata to use a typed TableProperties struct instead of a raw HashMap<String, String>, improving type safety and reducing allocations by returning references instead of reconstructing property maps.

Changes:

• Replaced HashMap<String, String> with TableProperties in TableMetadata
• Modified table_properties() to return a reference to the struct instead of a Result
• Added other field to TableProperties to store non-explicitly mapped properties
• Updated serialization/deserialization logic to handle the new structure

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
crates/iceberg/src/transaction/mod.rs	Updated to use reference-based API and clone when needed
crates/iceberg/src/spec/table_properties.rs	Added new field, traits, and constructor method
crates/iceberg/src/spec/table_metadata_builder.rs	Updated to use TableProperties struct and access other field
crates/iceberg/src/spec/table_metadata.rs	Changed internal field type and modified public API

Comments suppressed due to low confidence (3)

crates/iceberg/src/spec/table_metadata.rs:3893

• The test now calls .unwrap() on a method that no longer returns a Result, suggesting tests need to be updated. After the refactor, table_properties() returns &TableProperties directly, so the .unwrap() is no longer valid and would cause compilation failures.

        let props = metadata.table_properties().unwrap();

crates/iceberg/src/spec/table_metadata.rs:3940

• The test calls .unwrap() on a method that now returns &TableProperties instead of Result<TableProperties>, which would cause a compilation error.

        let props = metadata.table_properties().unwrap();

crates/iceberg/src/spec/table_metadata.rs:3973

• This test expects table_properties() to return an error for invalid property values, but the refactored API returns &TableProperties and the new() method silently uses defaults via unwrap_or_default(). This test would no longer compile and the error handling behavior it validates is lost.

        let err = metadata.table_properties().unwrap_err();

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The other field is set to a full clone of the input props HashMap, but this includes all properties (even the ones that were already parsed into struct fields). This creates unnecessary duplication and memory overhead. Consider filtering out the explicitly parsed properties before storing in other.

[Copilot]

The new() method silently swallows errors via unwrap_or_default(), which could hide parsing failures for invalid property values. This differs from the original behavior where table_properties() returned a Result. Consider returning Result<Self, anyhow::Error> to preserve error handling capabilities, or at minimum document that invalid values fall back to defaults.

Suggested change

	pub fn new(props: HashMap<String, String>) -> Self {
	Self::try_from(&props).unwrap_or_default()
	pub fn new(props: HashMap<String, String>) -> Result<Self, anyhow::Error> {
	Self::try_from(&props)

[Copilot]

Making the other field public exposes internal implementation details and allows external code to directly modify properties, potentially causing inconsistencies. Consider making this field private and providing accessor methods if needed.

Suggested change

	pub other: HashMap<String, String>,
	}
	impl TableProperties {
	other: HashMap<String, String>,
	}
	impl TableProperties {
	/// Returns a reference to the map of additional, non-standard table properties.
	pub fn other(&self) -> &HashMap<String, String> {
	&self.other
	}
	/// Returns a mutable reference to the map of additional, non-standard table properties.
	pub fn other_mut(&mut self) -> &mut HashMap<String, String> {
	&mut self.other
	}

[Copilot]

This clones the entire TableProperties struct (including the potentially large other HashMap) just to pass it to build_backoff(). Since build_backoff() only reads a few fields, passing a reference would be more efficient.

Suggested change

	let backoff = Self::build_backoff(table_props.clone())?;
	let backoff = Self::build_backoff(&table_props)?;