diff --git a/pyiceberg/catalog/glue.py b/pyiceberg/catalog/glue.py index 977876918c..a7d0683a07 100644 --- a/pyiceberg/catalog/glue.py +++ b/pyiceberg/catalog/glue.py @@ -147,15 +147,22 @@ def _construct_parameters( glue_table: Optional["TableTypeDef"] = None, prev_metadata_location: str | None = None, metadata_properties: Properties | None = None, + prev_metadata_properties: Properties | None = None, ) -> Properties: new_parameters = glue_table.get("Parameters", {}) if glue_table else {} + if prev_metadata_properties: + for key in prev_metadata_properties: + if not metadata_properties or key not in metadata_properties: + new_parameters.pop(key, None) new_parameters.update({TABLE_TYPE: ICEBERG.upper(), METADATA_LOCATION: metadata_location}) if prev_metadata_location: new_parameters[PREVIOUS_METADATA_LOCATION] = prev_metadata_location if metadata_properties: for key, value in metadata_properties.items(): - new_parameters[key] = str(value) + # Reserved parameters are managed by the catalog and must not be overwritten by table properties + if key not in (TABLE_TYPE, METADATA_LOCATION, PREVIOUS_METADATA_LOCATION): + new_parameters[key] = str(value) return new_parameters @@ -243,11 +250,18 @@ def _construct_table_input( metadata: TableMetadata, glue_table: Optional["TableTypeDef"] = None, prev_metadata_location: str | None = None, + prev_metadata_properties: Properties | None = None, ) -> "TableInputTypeDef": table_input: TableInputTypeDef = { "Name": table_name, "TableType": EXTERNAL_TABLE, - "Parameters": _construct_parameters(metadata_location, glue_table, prev_metadata_location, properties), + "Parameters": _construct_parameters( + metadata_location=metadata_location, + glue_table=glue_table, + prev_metadata_location=prev_metadata_location, + metadata_properties=properties, + prev_metadata_properties=prev_metadata_properties, + ), "StorageDescriptor": { "Columns": _to_columns(metadata), "Location": metadata.location, @@ -693,6 +707,7 @@ def commit_table( metadata=updated_staged_table.metadata, glue_table=current_glue_table, prev_metadata_location=current_table.metadata_location, + prev_metadata_properties=current_table.properties, ) self._update_glue_table( database_name=database_name, diff --git a/tests/catalog/test_glue.py b/tests/catalog/test_glue.py index fd476c978a..8943858b88 100644 --- a/tests/catalog/test_glue.py +++ b/tests/catalog/test_glue.py @@ -859,6 +859,68 @@ def test_commit_table_properties( assert table_info["Table"]["Parameters"]["test_c"] == "test_c" +@mock_aws +def test_commit_table_prunes_removed_properties( + _glue: boto3.client, + _bucket_initialize: None, + moto_endpoint_url: str, + table_schema_nested: Schema, + database_name: str, + table_name: str, +) -> None: + catalog_name = "glue" + identifier = (database_name, table_name) + test_catalog = GlueCatalog( + catalog_name, client=None, **{"s3.endpoint": moto_endpoint_url, "warehouse": f"s3://{BUCKET_NAME}"} + ) + test_catalog.create_namespace(namespace=database_name) + table = test_catalog.create_table( + identifier=identifier, schema=table_schema_nested, properties={"test_a": "test_a", "test_b": "test_b"} + ) + + transaction = table.transaction() + transaction.remove_properties("test_b") + transaction.commit_transaction() + + assert table.metadata.properties == {"test_a": "test_a"} + + table_info = _glue.get_table(DatabaseName=database_name, Name=table_name) + assert table_info["Table"]["Parameters"]["test_a"] == "test_a" + assert "test_b" not in table_info["Table"]["Parameters"] + + +@mock_aws +def test_commit_table_properties_do_not_overwrite_reserved_parameters( + _glue: boto3.client, + _bucket_initialize: None, + moto_endpoint_url: str, + table_schema_nested: Schema, + database_name: str, + table_name: str, +) -> None: + identifier = (database_name, table_name) + test_catalog = GlueCatalog("glue", client=None, **{"s3.endpoint": moto_endpoint_url, "warehouse": f"s3://{BUCKET_NAME}"}) + test_catalog.create_namespace(namespace=database_name) + table = test_catalog.create_table(identifier=identifier, schema=table_schema_nested) + previous_metadata_location = table.metadata_location + + table.transaction().set_properties( + table_type="bogus", + metadata_location="s3://bogus/metadata.json", + previous_metadata_location="s3://bogus/previous.metadata.json", + ).commit_transaction() + + parameters = _glue.get_table(DatabaseName=database_name, Name=table_name)["Table"]["Parameters"] + assert parameters["table_type"] == "ICEBERG" + assert parameters["metadata_location"] == table.metadata_location + assert parameters["previous_metadata_location"] == previous_metadata_location + assert test_catalog.load_table(identifier).metadata_location == table.metadata_location + + # Removing the properties must not prune the reserved parameters either + table.transaction().remove_properties("table_type", "metadata_location", "previous_metadata_location").commit_transaction() + assert test_catalog.load_table(identifier).metadata_location == table.metadata_location + + @mock_aws def test_commit_append_table_snapshot_properties( _bucket_initialize: None, moto_endpoint_url: str, table_schema_simple: Schema, database_name: str, table_name: str