You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "bharos (via GitHub)" <gi...@apache.org> on 2026/01/19 23:31:32 UTC
[PR] fix: preserve HMS table properties during commits [iceberg-python]
bharos opened a new pull request, #2927:
URL: https://github.com/apache/iceberg-python/pull/2927
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# 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:
1. Sets HMS properties via Hive client (simulating external systems)
2. Performs PyIceberg commit
3. 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706797220
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,118 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_preserves_hms_specific_properties(catalog: Catalog) -> None:
+ """Test that HMS-specific table properties are preserved during table commits.
+
+ This verifies that HMS-specific properties that are not managed by Iceberg
+ are preserved during commits, rather than being lost.
+
+ Regression test for: https://github.com/apache/iceberg-python/issues/2926
+ """
+ table = create_table(catalog)
+ hive_client: _HiveClient = _HiveClient(catalog.properties["uri"])
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # Add HMS-specific properties that aren't managed by Iceberg
+ hive_table.parameters["table_category"] = "production"
+ hive_table.parameters["data_owner"] = "data_team"
+ open_client.alter_table(TABLE_NAME[0], TABLE_NAME[1], hive_table)
+
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ assert hive_table.parameters.get("table_category") == "production"
+ assert hive_table.parameters.get("data_owner") == "data_team"
+
+ table.transaction().set_properties({"iceberg_property": "new_value"}).commit_transaction()
+
+ # Verify that HMS-specific properties are STILL present after commit
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # HMS-specific properties should be preserved
+ assert hive_table.parameters.get("table_category") == "production", (
+ "HMS property 'table_category' was lost during commit!"
+ )
+ assert hive_table.parameters.get("data_owner") == "data_team", "HMS property 'data_owner' was lost during commit!"
+ # Iceberg properties should also be present
+ assert hive_table.parameters.get("iceberg_property") == "new_value"
+
+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_iceberg_property_takes_precedence(catalog: Catalog) -> None:
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706929689
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,118 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
Review Comment:
done
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
+ new_iceberg_keys = set(updated_staged_table.properties.keys())
+ removed_keys = old_iceberg_keys - new_iceberg_keys
+
+ # 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}
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2713417831
##########
pyiceberg/catalog/hive.py:
##########
Review Comment:
we should double check glue and dynamo, any catalog that has this behavior of additional parameters that is now part of iceberg metadata
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
Review Comment:
nit: `new_iceberg_properties`
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ removed_keys = current_table.properties.keys() - updated_staged_table.properties.keys()
Review Comment:
nit: `deleted_iceberg_properties`
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
Review Comment:
i think we can add some documentation here about how there are 2 different property types, iceberg and hms. and the semantics of add/update/delete for each property type.
we can do this in a follow up
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ removed_keys = current_table.properties.keys() - updated_staged_table.properties.keys()
+
+ # Sync HMS parameters: Iceberg metadata is the source of truth, HMS parameters are
+ # a projection of Iceberg state plus any HMS-only properties.
+ # Start with existing HMS params, remove deleted Iceberg properties, then apply Iceberg values.
+ merged_params = dict(hive_table.parameters or {})
Review Comment:
nit: `existing_hms_parameters`
wdyt of this comment:
```
# Merge: preserve HMS-only properties, remove deleted Iceberg properties, apply new Iceberg properties
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2713390382
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ removed_keys = current_table.properties.keys() - updated_staged_table.properties.keys()
+
+ # Sync HMS parameters: Iceberg metadata is the source of truth, HMS parameters are
+ # a projection of Iceberg state plus any HMS-only properties.
+ # Start with existing HMS params, remove deleted Iceberg properties, then apply Iceberg values.
+ merged_params = dict(hive_table.parameters or {})
+ for key in removed_keys:
+ merged_params.pop(key, None)
+ merged_params.update(new_parameters)
+ hive_table.parameters = merged_params
+
# Update hive's schema and properties
hive_table.sd = _construct_hive_storage_descriptor(
updated_staged_table.schema(),
updated_staged_table.location(),
- property_as_bool(updated_staged_table.properties, HIVE2_COMPATIBLE, HIVE2_COMPATIBLE_DEFAULT),
+ property_as_bool(self.properties, HIVE2_COMPATIBLE, HIVE2_COMPATIBLE_DEFAULT),
Review Comment:
oo thanks for catching this bug, `HIVE2_COMPATIBLE` is a catalog property. so we should check in `self.properties`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2717906051
##########
pyiceberg/catalog/hive.py:
##########
Review Comment:
opened https://github.com/apache/iceberg-python/issues/2940 for tracking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2717708292
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,16 +551,29 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
Review Comment:
how about
```
# Note on table properties:
# - Iceberg table properties are stored in both HMS and Iceberg metadata JSON.
# - Updates are reflected in both locations
# - Existing HMS table properties (set by external systems like Hive/Spark) are preserved.
#
# While it is possible to modify HMS table properties through this API, it is not recommended:
# - New/Updated HMS table properties will also be stored in Iceberg metadata (even though it's HMS-specific)
# - HMS properties cannot be deleted since they are not visible to Iceberg
# - Mixing HMS-specific properties in Iceberg metadata can cause confusion
```
do you think this captures some of the items we talked about in this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#issuecomment-3785667414
lets get this in, i can raise a new pr with the suggested change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706510676
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
+ new_iceberg_keys = set(updated_staged_table.properties.keys())
+ removed_keys = old_iceberg_keys - new_iceberg_keys
+
+ # 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}
Review Comment:
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.
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
+ new_iceberg_keys = set(updated_staged_table.properties.keys())
+ removed_keys = old_iceberg_keys - new_iceberg_keys
+
+ # 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}
Review Comment:
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
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
Review Comment:
nit: can simplify to `current_table.properties.keys() - updated_staged_table.properties.keys()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#issuecomment-3770560913
Thanks for the fix. A few things to check for HMS properties that I can remember:
1. 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.
2. 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.
3. Things like `EXTERNAL`, `table_type`, `metadata_location` should always come from Iceberg, not be carried over from old HMS state.
4. Worth expanding the tests to test to check for removing a property and checking it is removed, and Iceberg value preservation.
CC: @kevinjqliu @Fokko
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#issuecomment-3785671970
Thanks for the PR @bharos and thanks for the review @Fokko @nssalian @geruh
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "nssalian (via GitHub)" <gi...@apache.org>.
nssalian commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706442030
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
+ new_iceberg_keys = set(updated_staged_table.properties.keys())
+ removed_keys = old_iceberg_keys - new_iceberg_keys
+
+ # 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}
Review Comment:
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
```
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,118 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_preserves_hms_specific_properties(catalog: Catalog) -> None:
+ """Test that HMS-specific table properties are preserved during table commits.
+
+ This verifies that HMS-specific properties that are not managed by Iceberg
+ are preserved during commits, rather than being lost.
+
+ Regression test for: https://github.com/apache/iceberg-python/issues/2926
+ """
+ table = create_table(catalog)
+ hive_client: _HiveClient = _HiveClient(catalog.properties["uri"])
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # Add HMS-specific properties that aren't managed by Iceberg
+ hive_table.parameters["table_category"] = "production"
+ hive_table.parameters["data_owner"] = "data_team"
+ open_client.alter_table(TABLE_NAME[0], TABLE_NAME[1], hive_table)
+
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ assert hive_table.parameters.get("table_category") == "production"
+ assert hive_table.parameters.get("data_owner") == "data_team"
+
+ table.transaction().set_properties({"iceberg_property": "new_value"}).commit_transaction()
+
+ # Verify that HMS-specific properties are STILL present after commit
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # HMS-specific properties should be preserved
+ assert hive_table.parameters.get("table_category") == "production", (
+ "HMS property 'table_category' was lost during commit!"
+ )
+ assert hive_table.parameters.get("data_owner") == "data_team", "HMS property 'data_owner' was lost during commit!"
+ # Iceberg properties should also be present
+ assert hive_table.parameters.get("iceberg_property") == "new_value"
+
+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_iceberg_property_takes_precedence(catalog: Catalog) -> None:
Review Comment:
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.
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,118 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
Review Comment:
Worth adding a test for property deletion to ensure it's not picked up from the old state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#issuecomment-3770606998
> Thanks for the fix. A few things to check for HMS properties that I can remember:
>
> 1. 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.
> 2. 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.
> 3. Things like `EXTERNAL`, `table_type`, `metadata_location` should always come from Iceberg, not be carried over from old HMS state.
> 4. 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2710040150
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
+ new_iceberg_keys = set(updated_staged_table.properties.keys())
+ removed_keys = old_iceberg_keys - new_iceberg_keys
+
+ # 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}
Review Comment:
I'm considering that we can do this refactor in a future PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2709742994
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,158 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_preserves_hms_specific_properties(catalog: Catalog) -> None:
+ """Test that HMS-specific table properties are preserved during table commits.
+
+ This verifies that HMS-specific properties that are not managed by Iceberg
+ are preserved during commits, rather than being lost.
+
+ Regression test for: https://github.com/apache/iceberg-python/issues/2926
+ """
+ table = create_table(catalog)
+ hive_client: _HiveClient = _HiveClient(catalog.properties["uri"])
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # Add HMS-specific properties that aren't managed by Iceberg
+ hive_table.parameters["table_category"] = "production"
+ hive_table.parameters["data_owner"] = "data_team"
+ open_client.alter_table(TABLE_NAME[0], TABLE_NAME[1], hive_table)
+
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ assert hive_table.parameters.get("table_category") == "production"
+ assert hive_table.parameters.get("data_owner") == "data_team"
+
+ table.transaction().set_properties({"iceberg_property": "new_value"}).commit_transaction()
+
+ # Verify that HMS-specific properties are STILL present after commit
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # HMS-specific properties should be preserved
+ assert hive_table.parameters.get("table_category") == "production", (
+ "HMS property 'table_category' was lost during commit!"
+ )
+ assert hive_table.parameters.get("data_owner") == "data_team", "HMS property 'data_owner' was lost during commit!"
+ # Iceberg properties should also be present
+ assert hive_table.parameters.get("iceberg_property") == "new_value"
+
+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_iceberg_property_deletion_not_restored_from_old_hms_state(catalog: Catalog) -> None:
Review Comment:
Nit: parameterizing these tests do not require parameterization since there is only one argument.
```suggestion
def test_iceberg_property_deletion_not_restored_from_old_hms_state(session_catalog_hive: Catalog) -> None:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706536421
##########
tests/integration/test_reads.py:
##########
@@ -135,6 +135,118 @@ def test_hive_properties(catalog: Catalog) -> None:
assert hive_table.parameters.get("abc") is None
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_preserves_hms_specific_properties(catalog: Catalog) -> None:
+ """Test that HMS-specific table properties are preserved during table commits.
+
+ This verifies that HMS-specific properties that are not managed by Iceberg
+ are preserved during commits, rather than being lost.
+
+ Regression test for: https://github.com/apache/iceberg-python/issues/2926
+ """
+ table = create_table(catalog)
+ hive_client: _HiveClient = _HiveClient(catalog.properties["uri"])
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # Add HMS-specific properties that aren't managed by Iceberg
+ hive_table.parameters["table_category"] = "production"
+ hive_table.parameters["data_owner"] = "data_team"
+ open_client.alter_table(TABLE_NAME[0], TABLE_NAME[1], hive_table)
+
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ assert hive_table.parameters.get("table_category") == "production"
+ assert hive_table.parameters.get("data_owner") == "data_team"
+
+ table.transaction().set_properties({"iceberg_property": "new_value"}).commit_transaction()
+
+ # Verify that HMS-specific properties are STILL present after commit
+ with hive_client as open_client:
+ hive_table = open_client.get_table(*TABLE_NAME)
+ # HMS-specific properties should be preserved
+ assert hive_table.parameters.get("table_category") == "production", (
+ "HMS property 'table_category' was lost during commit!"
+ )
+ assert hive_table.parameters.get("data_owner") == "data_team", "HMS property 'data_owner' was lost during commit!"
+ # Iceberg properties should also be present
+ assert hive_table.parameters.get("iceberg_property") == "new_value"
+
+
+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])
+def test_hive_iceberg_property_takes_precedence(catalog: Catalog) -> None:
Review Comment:
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:
1. We start our with the existing HMS params
2. Push all Iceberg properties into HMS
3. Remove old props
4. Then set all the iceberg properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#discussion_r2706796614
##########
pyiceberg/catalog/hive.py:
##########
@@ -551,11 +551,24 @@ def commit_table(
if hive_table and current_table:
# Table exists, update it.
- hive_table.parameters = _construct_parameters(
+ new_parameters = _construct_parameters(
metadata_location=updated_staged_table.metadata_location,
previous_metadata_location=current_table.metadata_location,
metadata_properties=updated_staged_table.properties,
)
+
+ # Detect properties that were removed from Iceberg metadata
+ old_iceberg_keys = set(current_table.properties.keys())
Review Comment:
Done, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix: preserve HMS table properties during commits [iceberg-python]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on PR #2927:
URL: https://github.com/apache/iceberg-python/pull/2927#issuecomment-3785846496
Thanks for the reviews and insightful comments everyone!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org