Repository navigation
Glue: Prune removed table properties from Glue Parameters - #4133
Queued
Ankitraj-builds wants to merge 2 commits into
Queued
Ankitraj-builds wants to merge 2 commits into
Ankitraj-builds wants to merge 2 commits into
Conversation
rambleraptor
left a comment
rambleraptor
left a comment
Collaborator
There was a problem hiding this comment.
quick nit. thanks for the PR!
| "TableType": EXTERNAL_TABLE, | ||
| "Parameters": _construct_parameters(metadata_location, glue_table, prev_metadata_location, properties), | ||
| "Parameters": _construct_parameters( | ||
| metadata_location, glue_table, prev_metadata_location, properties, prev_metadata_properties |
Collaborator
There was a problem hiding this comment.
Can you use named parameters for this? We've got a lot of properties with very similar names and ever so different orderings of parameters.
Author
There was a problem hiding this comment.
Done, switched to keyword arguments in the latest commit. Thanks for the review!
Fokko
approved these changes
Oct 22, 2026
Fokko
left a comment
Fokko
left a comment
Contributor
There was a problem hiding this comment.
Thanks for fixing this @Ankitraj-builds
Fokko
enabled auto-merge
October 22, 2026 21:14
rambleraptor
approved these changes
Oct 22, 2026
rambleraptor
left a comment
rambleraptor
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for that (very quick!) change
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3977
Rationale for this change
_construct_parametersinpyiceberg/catalog/glue.pystarts from the existing GlueParametersand only adds or overwrites keys. When a property is removed from the table, it stays in Glue forever, so Glue and the table properties drift apart.This change passes the table's previous properties into
_construct_parametersduringcommit_table. Any key that was in the previous properties but is not in the new ones is removed fromParameters. Glue keys that never came from Iceberg table properties are kept, which matches the Java Glue catalog behavior that @David-Banquet pointed out in the issue.The new argument is optional with a default of
None, socreate_table,register_tableand other callers are unchanged.Are these changes tested?
Yes. Added
test_commit_table_prunes_removed_propertiesintests/catalog/test_glue.py. It creates a table with two properties, removes one, commits, and checks that the removed key is gone from GlueParameterswhile the other one stays. The test fails onmainand passes with this change. All 53 tests intests/catalog/test_glue.pypass locally.Are there any user-facing changes?
Yes. After this change, properties removed from a Glue-backed table are also removed from the Glue table
Parameterson the next commit. Glue keys not managed by Iceberg are not touched.