Repository navigation
Glue: Prune removed table properties from Glue Parameters - #4133
Conversation
rambleraptor
left a comment
rambleraptor
left a comment
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 |
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.
There was a problem hiding this comment.
Done, switched to keyword arguments in the latest commit. Thanks for the review!
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks for fixing this @Ankitraj-builds
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Thanks for that (very quick!) change
|
Thanks @Fokko and @rambleraptor It looks like the merge queue removed this PR. The branch still merges cleanly with main and the Glue tests pass locally. Could you re-add it to the queue when you get a chance? |
A table property named table_type, metadata_location, or previous_metadata_location overwrote the catalog-managed Glue parameter of the same name, which could point Glue at a bogus metadata file and break load_table. Skip reserved keys when copying table properties, matching the Hive catalog. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
LGTM
I added a fix to guard against setting reserved keys (table_type, metadata_location, previous_metadata_location) in 3194838 (this PR)
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.Additionally, table properties named
table_type,metadata_locationorprevious_metadata_locationcan no longer overwrite the catalog-managed Glue parameters of the same name, which could breakload_table. Covered bytest_commit_table_properties_do_not_overwrite_reserved_parameters.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.