-
Notifications
You must be signed in to change notification settings - Fork 425
add hive catalog table properties docs and refactor #2941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds comprehensive documentation explaining the interaction between Iceberg table properties and HMS (Hive Metastore) table properties, along with a minor refactoring to improve variable naming clarity.
Changes:
- Added detailed documentation comments explaining how Iceberg and HMS properties are synchronized during table commits
- Refactored variable names to be more explicit (
new_parameters→new_iceberg_properties,removed_keys→deleted_iceberg_properties,merged_params→existing_hms_parameters) - Updated inline comments to better describe the merge operation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # - 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) |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment uses "it's" (contraction) but the rest of the documentation uses full words. For consistency and formality in code documentation, consider using "it is" instead of "it's".
| # - New/Updated HMS table properties will also be stored in Iceberg metadata (even though it's HMS-specific) | |
| # - New/Updated HMS table properties will also be stored in Iceberg metadata (even though it is HMS-specific) |
| # | ||
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be more explicit here like
HMS-native properties (set outside Iceberg) cannot be deleted since they are not visible to Iceberg
because in cases like below, it can actually delete HMS properties?
# Set via Iceberg
table.updateProperties().set("hive.table_prop", "true").commit()
# → Goes to BOTH Iceberg metadata AND HMS
# → Now it's "visible" to Iceberg (tracked in Iceberg metadata)
# Unset via Iceberg
table.updateProperties().remove("hive.table_prop").commit()
# → deleted_iceberg_properties = {"hive.table_prop"}
# → Removed from BOTH Iceberg metadata AND HMS ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea good point!
Fokko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying this @kevinjqliu 👍
Rationale for this change
Follow up to #2927
Add notes on updating iceberg table properties and hive table properties when using the hive catalog (HMS)
Minor refactor on variable naming to be more explicit
Are these changes tested?
Are there any user-facing changes?