fix: preserve HMS table properties during commits #2927

bharos · 2026-01-19T23:31:29Z

Rationale for this change

HMS-specific table properties (e.g., table_category from data contracts) were lost during PyIceberg commits because commit_table() replaced all HMS parameters instead of merging them.

Fixes: #2926

Are these changes tested?

Yes. Added test_hive_preserves_hms_specific_properties() that:

Sets HMS properties via Hive client (simulating external systems)
Performs PyIceberg commit
Verifies HMS properties are preserved

Are there any user-facing changes?

Bug fix only - HMS properties set outside PyIceberg are now preserved during commits. No API changes.

This change fixes HMS-specific table properties being lost during commits by merging parameters instead of replacing them. Fixes: apache#2926

nssalian · 2026-01-20T00:42:14Z

Thanks for the fix. A few things to check for HMS properties that I can remember:

If an external tool happens to set a property with the same name as an Iceberg-managed one, we need to make sure Iceberg's value should take predence.
If someone removes a property through Iceberg, make sure it actually gets deleted. Right now it might come back from the old HMS state after the merge.
Things like EXTERNAL, table_type, metadata_location should always come from Iceberg, not be carried over from old HMS state.
Worth expanding the tests to test to check for removing a property and checking it is removed, and Iceberg value preservation.
CC: @kevinjqliu @Fokko

bharos · 2026-01-20T01:11:02Z

Thanks for the fix. A few things to check for HMS properties that I can remember:

If an external tool happens to set a property with the same name as an Iceberg-managed one, we need to make sure Iceberg's value should take predence.

If someone removes a property through Iceberg, make sure it actually gets deleted. Right now it might come back from the old HMS state after the merge.

Things like EXTERNAL, table_type, metadata_location should always come from Iceberg, not be carried over from old HMS state.

Worth expanding the tests to test to check for removing a property and checking it is removed, and Iceberg value preservation.
CC: @kevinjqliu @Fokko

Makes sense. Added more tests to check these cases, also the code gives precedence to iceberg properties, PTAL and let me know if you find any issues. Thanks!

nssalian · 2026-01-20T01:20:10Z

pyiceberg/catalog/hive.py

+                    # Merge HMS parameters: preserve HMS-only properties (e.g., table_category) while
+                    # ensuring Iceberg controls its metadata. Start with HMS params, remove deleted
+                    # Iceberg properties, then update with new Iceberg values (which take precedence).
+                    updated_parameters = {k: v for k, v in hive_table.parameters.items() if k not in removed_keys}


While this looks fine, I think it would be cleaner if we started with the existing, removed and then updated to have the final params. Something like this:

merged_params = dict(hive_table.parameters) for key in removed_keys: merged_params.pop(key, None) merged_params.update(new_parameters) hive_table.parameters = merged_params

nssalian · 2026-01-20T01:38:58Z

tests/integration/test_reads.py

        assert hive_table.parameters.get("abc") is None


+@pytest.mark.integration


Worth adding a test for property deletion to ensure it's not picked up from the old state.

nssalian · 2026-01-20T01:40:11Z

tests/integration/test_reads.py

+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_iceberg_property_takes_precedence(catalog: Catalog) -> None:


I think this should be named better. It's more like Iceberg's metadata is the source of truth. CC: @kevinjqliu to see what the right expected behavior is.

This is a good call, this should be phrased as iceberg metadata is the source of truth, and HMS parameters are a projection of Iceberg state, plus any HMS-only properties that external tools may have set.

Looking at the Java implementation, they follow this order to sync properties:

We start our with the existing HMS params

Push all Iceberg properties into HMS

Remove old props

Then set all the iceberg properties

geruh · 2026-01-20T01:42:19Z

pyiceberg/catalog/hive.py

+                    # Merge HMS parameters: preserve HMS-only properties (e.g., table_category) while
+                    # ensuring Iceberg controls its metadata. Start with HMS params, remove deleted
+                    # Iceberg properties, then update with new Iceberg values (which take precedence).
+                    updated_parameters = {k: v for k, v in hive_table.parameters.items() if k not in removed_keys}


nit: params could be None here.

https://github.com/apache/iceberg/blob/369fe89138ca52ef9437fe657324c659889a8d88/hive-metastore/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java#L79

Not blocking as this solves the issue, but one suggestion would be to extract the merge logic into a small helper function like _merge_hms_parameters(). That way we could unit test without mocking the full commit flow, and document the semantics in one spot.

geruh · 2026-01-20T02:01:11Z

pyiceberg/catalog/hive.py

                    )
+
+                    # Detect properties that were removed from Iceberg metadata
+                    old_iceberg_keys = set(current_table.properties.keys())


nit: can simplify to current_table.properties.keys() - updated_staged_table.properties.keys()

Done, thanks

bharos force-pushed the fix/hive-preserve-hms-properties branch 2 times, most recently from 96b6185 to 46ee7f7 Compare January 20, 2026 00:08

fix: preserve HMS table properties during commits

be4e2a7

This change fixes HMS-specific table properties being lost during commits by merging parameters instead of replacing them. Fixes: apache#2926

bharos force-pushed the fix/hive-preserve-hms-properties branch from 46ee7f7 to be4e2a7 Compare January 20, 2026 00:28

Add more tests, comment

ae8a024

bharos force-pushed the fix/hive-preserve-hms-properties branch from a9fbb1c to ae8a024 Compare January 20, 2026 00:54

bharos marked this pull request as ready for review January 20, 2026 01:11

nssalian reviewed Jan 20, 2026

View reviewed changes

geruh reviewed Jan 20, 2026

View reviewed changes

Address comments

291311f

bharos force-pushed the fix/hive-preserve-hms-properties branch from 5b2939b to 291311f Compare January 20, 2026 05:17

		assert hive_table.parameters.get("abc") is None


		@pytest.mark.integration

fix: preserve HMS table properties during commits #2927

Are you sure you want to change the base?

fix: preserve HMS table properties during commits #2927

Conversation

bharos commented Jan 19, 2026

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

nssalian commented Jan 20, 2026

Uh oh!

bharos commented Jan 20, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants