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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
