You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aajisaka (via GitHub)" <gi...@apache.org> on 2024/04/22 07:55:53 UTC
[PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
aajisaka opened a new pull request, #10199:
URL: https://github.com/apache/iceberg/pull/10199
## 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
Not tested as it's going to be a bit complex to add description to the existing table in Iceberg tests. Any idea is welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1585860715
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -322,6 +322,7 @@ void persistGlueTable(
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
+ .description(glueTable.description())
Review Comment:
I think the `comment` should be honored. I'll update the patch to
1. If `comment` exists, `comment` is applied.
1. If `comment` doesn't exist, the existing description is retained.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1599418688
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -206,6 +208,15 @@ public void testUpdateTable() {
.isEqualTo("EXTERNAL_TABLE");
assertThat(response.table().storageDescriptor().columns()).hasSameSizeAs(schema.columns());
assertThat(response.table().partitionKeys()).hasSameSizeAs(partitionSpec.fields());
+ assertThat(response.table().description()).isEqualTo(description);
+
+ // Make sure adding a comment updates the existing table description
+ String comment = "Test comment";
Review Comment:
Make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2111361552
Thank you @geruh @amogh-jahagirdar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1599192463
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,11 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Keep the existing table description
+ .description(glueTable.description())
+ // This overwrites the existing table description if there's a comment like
Review Comment:
> This overwrites the existing table description
You mean `applyMutation` overwrites right? I'd just explicitly say that on the comment in 319
```
// Explicitly keep the existing table description since `applyMutation` will clear the table description
```
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -206,6 +208,15 @@ public void testUpdateTable() {
.isEqualTo("EXTERNAL_TABLE");
assertThat(response.table().storageDescriptor().columns()).hasSameSizeAs(schema.columns());
assertThat(response.table().partitionKeys()).hasSameSizeAs(partitionSpec.fields());
+ assertThat(response.table().description()).isEqualTo(description);
+
+ // Make sure adding a comment updates the existing table description
+ String comment = "Test comment";
Review Comment:
Nit: I think if you rename the variable you can probably get rid of the inline comment since it'll be clear.
Maybe just call it `updatedDescription` or `updatedComment` and then you won't need the inline comment since the test is clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2072339019
Thank you for your suggestion @geruh
I'll test this patch manually and update the class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2076512208
I ran the integration test using my AWS account and the `testUpdateTable()` was successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2111348073
Awesome work @aajisaka, looks good to me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2070875152
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.
https://github.com/apache/iceberg/blob/main/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2084524941
Hi @geruh would you review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2100651509
Hi @geruh @amogh-jahagirdar would you review the latest change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "geruh (via GitHub)" <gi...@apache.org>.
geruh commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1585580662
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -322,6 +322,7 @@ void persistGlueTable(
.name(tableName)
.tableType(GLUE_EXTERNAL_TABLE_TYPE)
.parameters(parameters)
+ .description(glueTable.description())
Review Comment:
Looks good @aajisaka this is great!
Recently we added support for setting the glue description via the `comment` table property in #9530. What are your thoughts on leveraging this table property to set the Glue description. This would allow a user to perform an ALTER statement to update the glue description as well.
```
ALTER TABLE prod.db.sample SET TBLPROPERTIES (
'comment' = 'A table description.'
)
```
cc: @amogh-jahagirdar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1599418533
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,11 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Keep the existing table description
+ .description(glueTable.description())
+ // This overwrites the existing table description if there's a comment like
Review Comment:
> You mean applyMutation overwrites right?
Yes.
> // Explicitly keep the existing table description since `applyMutation` will clear the table description
This is not right. `applyMutation` won't clear the table description, it overwrites the description only if the comment is specified by the user as below:
```
ALTER TABLE prod.db.sample SET TBLPROPERTIES ('comment' = 'A table description.')
```
I want to explain why I need to call `.description(glueTable.description())` first and call `applyMutation` second. If the order is reversed, user cannot update the table description by using the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#issuecomment-2076564061
Also, I built the jars and manually tested on AWS Glue as a Spark runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #10199:
URL: https://github.com/apache/iceberg/pull/10199
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1600313072
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,9 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Call description earlier than applyMutation to allow applyMutation to
+ // overwrite the description if the description is specified in the query
Review Comment:
Thanks. Updated the comment as you suggested
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1599192267
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,11 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Keep the existing table description
+ .description(glueTable.description())
+ // This overwrites the existing table description if there's a comment like
+ // ALTER TABLE prod.db.sample SET TBLPROPERTIES (
+ // 'comment' = 'A table description.')
Review Comment:
> This overwrites the existing table description
You mean `applyMutation` overwrites right? I'd just explicitly say that
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,11 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Keep the existing table description
+ .description(glueTable.description())
+ // This overwrites the existing table description if there's a comment like
+ // ALTER TABLE prod.db.sample SET TBLPROPERTIES (
+ // 'comment' = 'A table description.')
Review Comment:
> This overwrites the existing table description
You mean `applyMutation` overwrites right? I'd just explicitly say that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "aajisaka (via GitHub)" <gi...@apache.org>.
aajisaka commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1599419966
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,11 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Keep the existing table description
+ .description(glueTable.description())
+ // This overwrites the existing table description if there's a comment like
Review Comment:
Updated the comment in the latest change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1600221414
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,9 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Call description earlier than applyMutation to allow applyMutation to
+ // overwrite the description if the description is specified in the query
Review Comment:
```
Call description before applyMutation so that applyMutation overwrites the description with the one specified in the query
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] AWS: Retain Glue Catalog table description after updating Iceberg table [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #10199:
URL: https://github.com/apache/iceberg/pull/10199#discussion_r1600221414
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -316,6 +316,9 @@ void persistGlueTable(
.skipArchive(awsProperties.glueCatalogSkipArchive())
.tableInput(
TableInput.builder()
+ // Call description earlier than applyMutation to allow applyMutation to
+ // overwrite the description if the description is specified in the query
Review Comment:
```
Call description before applyMutation so that applyMutation overwrites the description with the comment specified in the query
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org