-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
AWS: Retain Glue Catalog table description after updating Iceberg table #10199
Conversation
Awesome, thanks for contributing this @aajisaka! Manually did this change work? For adding some tests, if the unit test logic in this module is difficult. I'd suggest leveraging this modules integration test workflow to update an created table with the glue client directly. |
Thank you for your suggestion @geruh I'll test this patch manually and update the class. |
I ran the integration test using my AWS account and the
|
Also, I built the jars and manually tested on AWS Glue as a Spark runtime. |
Hi @geruh would you review this PR? |
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
Outdated
Show resolved
Hide resolved
Hi @geruh @amogh-jahagirdar would you review the latest change? |
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.
Overall looks great @aajisaka, just some nits and then I think it's good to merge!
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java
Outdated
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java
Outdated
Show resolved
Hide resolved
Awesome work @aajisaka, looks good to me! |
Thank you @geruh @amogh-jahagirdar |
Problem
In AWS Glue Catalog, user can set arbitrary description to the table and its columns via Web UI or even from Amazon Athena. However, the descriptions will be removed after upgrading the table.
What this PR do
This patch is to keep the original table description when calling
UpdateTable
API. Keeping column description is hard and I think we can open a different PR to do this.Testing
Manually tested. Also, added an integration test and it passed on my environment.