refactor: replace HashMap with TableProperties in TableMetadata (#2028) by vigneshsiva11 · Pull Request #2037 · apache/iceberg-rust

vigneshsiva11 · 2026-01-16T12:20:12Z

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.

…he#2028)

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 · 2026-01-16T12:22:50Z

crates/iceberg/src/spec/table_properties.rs

                TableProperties::PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED,
                TableProperties::PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED_DEFAULT,
            )?,
+            other: props.clone(),


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 · 2026-01-16T12:22:50Z

crates/iceberg/src/spec/table_properties.rs

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


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 · 2026-01-16T12:22:51Z

crates/iceberg/src/spec/table_properties.rs

+    pub other: HashMap<String, String>,
 }

 impl TableProperties {


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 · 2026-01-16T12:22:51Z

crates/iceberg/src/transaction/mod.rs

+        let table_props = self.table.metadata().table_properties();

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


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)?;

pratik0316 · 2026-01-16T17:39:17Z

crates/iceberg/src/spec/table_properties.rs

    pub write_datafusion_fanout_enabled: bool,
+    /// Any other properties that are not explicitly captured in named fields.
+    #[serde(flatten)]
+    pub other: HashMap<String, String>,


This field name is not very descriptive to what it actually holds

…er review

liurenjie1024

Thanks @vigneshsiva11 , just finished first round of review.

liurenjie1024 · 2026-01-19T05:54:47Z

crates/iceberg/src/spec/table_metadata.rs

@@ -358,16 +357,13 @@ impl TableMetadata {
    /// Returns properties of table.
    #[inline]
    pub fn properties(&self) -> &HashMap<String, String> {


I think we should remove this method.

liurenjie1024 · 2026-01-19T05:58:37Z

crates/iceberg/src/spec/table_metadata_builder.rs

                ), // Overwritten immediately by add_default_partition_spec
                default_partition_type: StructType::new(vec![]),
                last_partition_id: UNPARTITIONED_LAST_ASSIGNED_ID,
-                properties: HashMap::new(),


I think in TableMetadataBuilder, we should still use HashMap<String, String> given it's a mutable api. We could add an extra field in TableMetadataBuilder.

liurenjie1024 · 2026-01-19T06:01:11Z

crates/iceberg/src/spec/table_properties.rs

    pub write_datafusion_fanout_enabled: bool,
+    /// Any other properties that are not explicitly captured in named fields.
+    #[serde(flatten)]
+    pub additional_properties: HashMap<String, String>,


I don't think we need this property.

liurenjie1024 · 2026-01-19T06:01:39Z

crates/iceberg/src/spec/table_properties.rs


 /// TableProperties that contains the properties of a table.
-#[derive(Debug)]
+#[derive(Debug, Clone, PartialEq, Eq, Default, serde::Serialize, serde::Deserialize)]


We have dedicated ser/de for TableMetadata, we should not add it here.

liurenjie1024 · 2026-01-19T06:02:55Z

crates/iceberg/src/spec/table_properties.rs

    pub const PROPERTY_SNAPSHOT_COUNT: &str = "snapshot-count";
+    /// Creates a new TableProperties from a HashMap of raw strings.
+    pub fn new(props: HashMap<String, String>) -> Self {
+        Self::try_from(&props).unwrap_or_default()


We should not unwrap, just implement TryFrom.

Standing-Man · 2026-01-28T13:46:22Z

Hi @liurenjie1024, Wouldn’t it be more convenient to use HashMap<String, String> directly in TableProperties? I think the current design might not be ideal. Using HashMap directly would require enforcing restrictions on the keys.

refactor: replace HashMap with TableProperties in TableMetadata (apac…

a0b53f8

…he#2028)

Copilot AI review requested due to automatic review settings January 16, 2026 12:20

Copilot started reviewing on behalf of vigneshsiva11 January 16, 2026 12:20 View session

Copilot AI reviewed Jan 16, 2026

View reviewed changes

Vignesh S added 3 commits January 16, 2026 12:54

fix: resolve integration errors and add serde flattening

c8e2003

style: fix mixed indentation in datafusion and location_generator

4af9794

fix: resolve compilation errors and structure test modules

cfd42b7

pratik0316 reviewed Jan 16, 2026

View reviewed changes

Vignesh S added 2 commits January 17, 2026 05:39

style: fix trailing whitespace at line 2043 for rustfmt

ffbb917

refactor: rename TableProperties 'other' to 'additional_properties' p…

849740a

…er review

liurenjie1024 reviewed Jan 19, 2026

View reviewed changes

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

-    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
+    }

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

Conversation

vigneshsiva11 commented Jan 16, 2026

Which issue does this PR close?

What changes are included in this PR?

Key Modifications:

Are these changes tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

pratik0316 Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Jan 19, 2026

Choose a reason for hiding this comment

Uh oh!

Standing-Man commented Jan 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants