AWS: Support setting description for Glue table (apache#9530)

adnanhemani · Jan 30, 2024 · 796b951 · 796b951
1 parent 7ede4bc
commit 796b951
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 12 deletions.
diff --git a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogNamespace.java b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogNamespace.java
@@ -51,7 +51,7 @@ public void testCreateNamespace() {
         .hasMessageContaining("not found");
     Map<String, String> properties =
         ImmutableMap.of(
-            IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY,
+            IcebergToGlueConverter.GLUE_DESCRIPTION_KEY,
             "description",
             IcebergToGlueConverter.GLUE_DB_LOCATION_KEY,
             "s3://location",
@@ -117,7 +117,7 @@ public void testNamespaceProperties() {
     properties.put("key", "val");
     properties.put("key2", "val2");
     properties.put(IcebergToGlueConverter.GLUE_DB_LOCATION_KEY, "s3://test");
-    properties.put(IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY, "description");
+    properties.put(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, "description");
     glueCatalog.setProperties(Namespace.of(namespace), properties);
     Database database =
         glue.getDatabase(GetDatabaseRequest.builder().name(namespace).build()).database();
@@ -133,7 +133,7 @@ public void testNamespaceProperties() {
         Sets.newHashSet(
             "key",
             IcebergToGlueConverter.GLUE_DB_LOCATION_KEY,
-            IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY));
+            IcebergToGlueConverter.GLUE_DESCRIPTION_KEY));
     database = glue.getDatabase(GetDatabaseRequest.builder().name(namespace).build()).database();
     Assert.assertFalse(database.parameters().containsKey("key"));
     Assert.assertTrue(database.parameters().containsKey("key2"));
@@ -144,7 +144,7 @@ public void testNamespaceProperties() {
     properties = Maps.newHashMap();
     properties.put("key", "val");
     properties.put(IcebergToGlueConverter.GLUE_DB_LOCATION_KEY, "s3://test2");
-    properties.put(IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY, "description2");
+    properties.put(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, "description2");
     glueCatalog.setProperties(Namespace.of(namespace), properties);
     database = glue.getDatabase(GetDatabaseRequest.builder().name(namespace).build()).database();
     Assert.assertTrue(database.parameters().containsKey("key"));

diff --git a/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java b/aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java
@@ -73,8 +73,14 @@ public class TestGlueCatalogTable extends GlueTestBase {
   public void testCreateTable() {
     String namespace = createNamespace();
     String tableName = getRandomName();
+    String tableDescription = "Test table";
+    Map<String, String> tableProperties =
+        ImmutableMap.<String, String>builder()
+            .putAll(tableLocationProperties)
+            .put(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, tableDescription)
+            .build();
     glueCatalog.createTable(
-        TableIdentifier.of(namespace, tableName), schema, partitionSpec, tableLocationProperties);
+        TableIdentifier.of(namespace, tableName), schema, partitionSpec, tableProperties);
     // verify table exists in Glue
     GetTableResponse response =
         glue.getTable(GetTableRequest.builder().databaseName(namespace).name(tableName).build());
@@ -105,6 +111,9 @@ public void testCreateTable() {
     Table table = glueCatalog.loadTable(TableIdentifier.of(namespace, tableName));
     Assert.assertEquals(partitionSpec, table.spec());
     Assert.assertEquals(schema.toString(), table.schema().toString());
+    Assert.assertEquals(
+        tableDescription, table.properties().get(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY));
+    Assert.assertEquals(tableDescription, response.table().description());
   }
 
   @Test

diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java b/aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java
@@ -521,7 +521,7 @@ public Map<String, String> loadNamespaceMetadata(Namespace namespace)
       }
 
       if (database.description() != null) {
-        result.put(IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY, database.description());
+        result.put(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, database.description());
       }
 
       LOG.debug("Loaded metadata for namespace {} found {}", namespace, result);

diff --git a/aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java b/aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java
@@ -23,6 +23,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
@@ -59,7 +60,8 @@ private IcebergToGlueConverter() {}
   private static final Pattern GLUE_DB_PATTERN = Pattern.compile("^[a-z0-9_]{1,252}$");
   private static final Pattern GLUE_TABLE_PATTERN = Pattern.compile("^[a-z0-9_]{1,255}$");
   public static final String GLUE_DB_LOCATION_KEY = "location";
-  public static final String GLUE_DB_DESCRIPTION_KEY = "comment";
+  // Utilized for defining descriptions at both the Glue database and table levels.
+  public static final String GLUE_DESCRIPTION_KEY = "comment";
   public static final String ICEBERG_FIELD_ID = "iceberg.field.id";
   public static final String ICEBERG_FIELD_OPTIONAL = "iceberg.field.optional";
   public static final String ICEBERG_FIELD_CURRENT = "iceberg.field.current";
@@ -150,7 +152,7 @@ static DatabaseInput toDatabaseInput(
     Map<String, String> parameters = Maps.newHashMap();
     metadata.forEach(
         (k, v) -> {
-          if (GLUE_DB_DESCRIPTION_KEY.equals(k)) {
+          if (GLUE_DESCRIPTION_KEY.equals(k)) {
             builder.description(v);
           } else if (GLUE_DB_LOCATION_KEY.equals(k)) {
             builder.locationUri(v);
@@ -218,16 +220,20 @@ static String getTableName(TableIdentifier tableIdentifier, boolean skipNameVali
   static void setTableInputInformation(
       TableInput.Builder tableInputBuilder, TableMetadata metadata) {
     try {
+      Map<String, String> properties = metadata.properties();
       StorageDescriptor.Builder storageDescriptor = StorageDescriptor.builder();
       if (!SET_ADDITIONAL_LOCATIONS.isNoop()) {
         SET_ADDITIONAL_LOCATIONS.invoke(
             storageDescriptor,
             ADDITIONAL_LOCATION_PROPERTIES.stream()
-                .map(metadata.properties()::get)
+                .map(properties::get)
                 .filter(Objects::nonNull)
                 .collect(Collectors.toSet()));
       }
 
+      Optional.ofNullable(properties.get(GLUE_DESCRIPTION_KEY))
+          .ifPresent(tableInputBuilder::description);
+
       tableInputBuilder.storageDescriptor(
           storageDescriptor.location(metadata.location()).columns(toColumns(metadata)).build());
     } catch (RuntimeException e) {

diff --git a/aws/src/test/java/org/apache/iceberg/aws/glue/TestIcebergToGlueConverter.java b/aws/src/test/java/org/apache/iceberg/aws/glue/TestIcebergToGlueConverter.java
@@ -100,7 +100,7 @@ public void testSkipTableNameValidation() {
   public void testToDatabaseInput() {
     Map<String, String> properties =
         ImmutableMap.of(
-            IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY,
+            IcebergToGlueConverter.GLUE_DESCRIPTION_KEY,
             "description",
             IcebergToGlueConverter.GLUE_DB_LOCATION_KEY,
             "s3://location",
@@ -132,8 +132,7 @@ public void testToDatabaseInputNoParameter() {
   @Test
   public void testToDatabaseInputEmptyLocation() {
     Map<String, String> properties =
-        ImmutableMap.of(
-            IcebergToGlueConverter.GLUE_DB_DESCRIPTION_KEY, "description", "key", "val");
+        ImmutableMap.of(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, "description", "key", "val");
     DatabaseInput databaseInput =
         IcebergToGlueConverter.toDatabaseInput(Namespace.of("ns"), properties, false);
     Assertions.assertThat(databaseInput.locationUri()).as("Location should not be set").isNull();
@@ -289,4 +288,27 @@ public void testSetTableInputInformationWithRemovedColumns() {
         .as("Columns should match")
         .isEqualTo(expectedTableInput.storageDescriptor().columns());
   }
+
+  @Test
+  public void testSetTableDescription() {
+    String tableDescription = "hello world!";
+    Map<String, String> tableProperties =
+        ImmutableMap.<String, String>builder()
+            .putAll((tableLocationProperties))
+            .put(IcebergToGlueConverter.GLUE_DESCRIPTION_KEY, tableDescription)
+            .build();
+    TableInput.Builder actualTableInputBuilder = TableInput.builder();
+    Schema schema =
+        new Schema(Types.NestedField.required(1, "x", Types.StringType.get(), "comment1"));
+    TableMetadata tableMetadata =
+        TableMetadata.newTableMetadata(
+            schema, PartitionSpec.unpartitioned(), "s3://test", tableProperties);
+
+    IcebergToGlueConverter.setTableInputInformation(actualTableInputBuilder, tableMetadata);
+    TableInput actualTableInput = actualTableInputBuilder.build();
+
+    Assertions.assertThat(actualTableInput.description())
+        .as("description should match")
+        .isEqualTo(tableDescription);
+  }
 }