[apache#598] improvement(api): introduce UpdateComment in SchemaChang…

…e interface to update comment of schema
SteNicholas · Dec 20, 2023 · c7589cf · c7589cf
1 parent 9dddd0c
commit c7589cf
Show file tree

Hide file tree

Showing 10 changed files with 204 additions and 47 deletions.
diff --git a/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java b/api/src/main/java/com/datastrato/gravitino/rel/SchemaChange.java
@@ -23,9 +23,22 @@
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 
-/** NamespaceChange class to set the property and value pairs for the namespace. */
+/**
+ * The SchemaChange interface defines the public API for managing schemas in a catalog. If the
+ * catalog implementation supports schemas, it must implement this interface.
+ */
 public interface SchemaChange {
 
+  /**
+   * SchemaChange class to update the comment for the schema.
+   *
+   * @param newComment The new comment to update.
+   * @return The SchemaChange object.
+   */
+  static SchemaChange updateComment(String newComment) {
+    return new UpdateComment(newComment);
+  }
+
   /**
    * SchemaChange class to set the property and value pairs for the schema.
    *
@@ -47,6 +60,22 @@ static SchemaChange removeProperty(String property) {
     return new RemoveProperty(property);
   }
 
+  /** A SchemaChange to update a schema's comment. */
+  @EqualsAndHashCode
+  @Getter
+  final class UpdateComment implements SchemaChange {
+    private final String newComment;
+
+    private UpdateComment(String newComment) {
+      this.newComment = newComment;
+    }
+  }
+
+  /**
+   * A SchemaChange to set a schema property.
+   *
+   * <p>If the property already exists, it must be replaced with the new value.
+   */
   @Getter
   @EqualsAndHashCode
   final class SetProperty implements SchemaChange {
@@ -59,6 +88,11 @@ private SetProperty(String property, String value) {
     }
   }
 
+  /**
+   * A SchemaChange to remove a schema property.
+   *
+   * <p>If the property does not exist, the change should succeed.
+   */
   @Getter
   @EqualsAndHashCode
   final class RemoveProperty implements SchemaChange {

diff --git a/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java b/api/src/test/java/com/datastrato/gravitino/TestSchemaChange.java
@@ -12,10 +12,19 @@
 import com.datastrato.gravitino.rel.SchemaChange;
 import com.datastrato.gravitino.rel.SchemaChange.RemoveProperty;
 import com.datastrato.gravitino.rel.SchemaChange.SetProperty;
+import com.datastrato.gravitino.rel.SchemaChange.UpdateComment;
 import org.junit.jupiter.api.Test;
 
 public class TestSchemaChange {
 
+  @Test
+  public void testUpdateComment() {
+    String comment = "New comment";
+    UpdateComment change = (UpdateComment) SchemaChange.updateComment(comment);
+
+    assertEquals(comment, change.getNewComment());
+  }
+
   @Test
   void testSetProperty() {
     String property = "Jam";
@@ -87,4 +96,29 @@ void testSetPropertyNotEqualsAndHashCode() {
     assertFalse(changeB.equals(changeA));
     assertNotEquals(changeA.hashCode(), changeB.hashCode());
   }
+
+  @Test
+  void testUpdateCommentEqualsAndHashCode() {
+    String commentA = "a comment";
+    UpdateComment changeA = (UpdateComment) SchemaChange.updateComment(commentA);
+    String commentB = "a comment";
+    UpdateComment changeB = (UpdateComment) SchemaChange.updateComment(commentB);
+
+    assertTrue(changeA.equals(changeB));
+    assertTrue(changeB.equals(changeA));
+    assertEquals(changeA.hashCode(), changeB.hashCode());
+  }
+
+  @Test
+  void testUpdateCommentNotEqualsAndHashCode() {
+    String commentA = "a comment";
+    UpdateComment changeA = (UpdateComment) SchemaChange.updateComment(commentA);
+    String commentB = "a new comment";
+    UpdateComment changeB = (UpdateComment) SchemaChange.updateComment(commentB);
+
+    assertFalse(changeA.equals(null));
+    assertFalse(changeA.equals(changeB));
+    assertFalse(changeB.equals(changeA));
+    assertNotEquals(changeA.hashCode(), changeB.hashCode());
+  }
 }
diff --git a/...talog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java b/...talog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogOperations.java
@@ -283,8 +283,13 @@ public HiveSchema alterSchema(NameIdentifier ident, SchemaChange... changes)
           ident.name(),
           properties.keySet());
 
+      boolean commentUpdated = false;
+      String comment = null;
       for (SchemaChange change : changes) {
-        if (change instanceof SchemaChange.SetProperty) {
+        if (change instanceof SchemaChange.UpdateComment) {
+          commentUpdated = true;
+          comment = ((SchemaChange.UpdateComment) change).getNewComment();
+        } else if (change instanceof SchemaChange.SetProperty) {
           properties.put(
               ((SchemaChange.SetProperty) change).getProperty(),
               ((SchemaChange.SetProperty) change).getValue());
@@ -299,6 +304,9 @@ public HiveSchema alterSchema(NameIdentifier ident, SchemaChange... changes)
       // alter the hive database parameters
       Database alteredDatabase = database.deepCopy();
       alteredDatabase.setParameters(properties);
+      if (commentUpdated) {
+        alteredDatabase.setDescription(comment);
+      }
 
       clientPool.run(
           client -> {

diff --git a/...ain/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/...ain/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
@@ -246,8 +246,11 @@ public IcebergSchema alterSchema(NameIdentifier ident, SchemaChange... changes)
       List<String> removals = new ArrayList<>();
       Map<String, String> updates = new HashMap<>();
       Map<String, String> resultProperties = new HashMap<>(metadata);
+      String comment = null;
       for (SchemaChange change : changes) {
-        if (change instanceof SchemaChange.SetProperty) {
+        if (change instanceof SchemaChange.UpdateComment) {
+          comment = ((SchemaChange.UpdateComment) change).getNewComment();
+        } else if (change instanceof SchemaChange.SetProperty) {
           String key = ((SchemaChange.SetProperty) change).getProperty();
           String val = ((SchemaChange.SetProperty) change).getValue();
           updates.put(key, val);
@@ -261,10 +264,12 @@ public IcebergSchema alterSchema(NameIdentifier ident, SchemaChange... changes)
         }
       }
 
-      String comment =
-          Optional.of(response.properties())
-              .map(map -> map.get(IcebergSchemaPropertiesMetadata.COMMENT))
-              .orElse(null);
+      comment =
+          Optional.ofNullable(comment)
+              .orElse(
+                  Optional.of(response.properties())
+                      .map(map -> map.get(IcebergSchemaPropertiesMetadata.COMMENT))
+                      .orElse(null));
       IcebergSchema icebergSchema =
           new IcebergSchema.Builder()
               .withName(ident.name())

diff --git a/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java b/clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java
@@ -99,7 +99,11 @@ static CatalogUpdateRequest toCatalogUpdateRequest(CatalogChange change) {
   }
 
   static SchemaUpdateRequest toSchemaUpdateRequest(SchemaChange change) {
-    if (change instanceof SchemaChange.SetProperty) {
+    if (change instanceof SchemaChange.UpdateComment) {
+      return new SchemaUpdateRequest.UpdateSchemaCommentRequest(
+          ((SchemaChange.UpdateComment) change).getNewComment());
+
+    } else if (change instanceof SchemaChange.SetProperty) {
       return new SchemaUpdateRequest.SetSchemaPropertyRequest(
           ((SchemaChange.SetProperty) change).getProperty(),
           ((SchemaChange.SetProperty) change).getValue());

diff --git a/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java b/clients/client-java/src/test/java/com/datastrato/gravitino/client/TestRelationalCatalog.java
@@ -235,6 +235,16 @@ public void testLoadSchema() throws JsonProcessingException {
     Assertions.assertTrue(ex1.getMessage().contains("schema not found"));
   }
 
+  @Test
+  public void testUpdateSchemaComment() throws JsonProcessingException {
+    NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, "schema1");
+    SchemaUpdateRequest.UpdateSchemaCommentRequest req =
+        new SchemaUpdateRequest.UpdateSchemaCommentRequest("comment");
+    SchemaDTO expectedSchema = createMockSchema("schema1", "comment", ImmutableMap.of("k1", "v1"));
+
+    testAlterSchema(ident, req, expectedSchema);
+  }
+
   @Test
   public void testSetSchemaProperty() throws JsonProcessingException {
     NameIdentifier ident = NameIdentifier.of(metalakeName, catalogName, "schema1");

diff --git a/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java b/common/src/main/java/com/datastrato/gravitino/dto/requests/SchemaUpdateRequest.java
@@ -19,6 +19,9 @@
 @JsonIgnoreProperties(ignoreUnknown = true)
 @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY)
 @JsonSubTypes({
+  @JsonSubTypes.Type(
+      value = SchemaUpdateRequest.UpdateSchemaCommentRequest.class,
+      name = "updateComment"),
   @JsonSubTypes.Type(
       value = SchemaUpdateRequest.SetSchemaPropertyRequest.class,
       name = "setProperty"),
@@ -30,6 +33,35 @@ public interface SchemaUpdateRequest extends RESTRequest {
 
   SchemaChange schemaChange();
 
+  @EqualsAndHashCode
+  @ToString
+  class UpdateSchemaCommentRequest implements SchemaUpdateRequest {
+
+    @Getter
+    @JsonProperty("newComment")
+    private final String newComment;
+
+    public UpdateSchemaCommentRequest(String newComment) {
+      this.newComment = newComment;
+    }
+
+    public UpdateSchemaCommentRequest() {
+      this(null);
+    }
+
+    @Override
+    public void validate() throws IllegalArgumentException {
+      Preconditions.checkArgument(
+          StringUtils.isNotBlank(newComment),
+          "\"newComment\" field is required and cannot be empty");
+    }
+
+    @Override
+    public SchemaChange schemaChange() {
+      return SchemaChange.updateComment(newComment);
+    }
+  }
+
   @EqualsAndHashCode
   @ToString
   class SetSchemaPropertyRequest implements SchemaUpdateRequest {

diff --git a/...t/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java b/...t/src/test/java/com/datastrato/gravitino/integration/test/catalog/hive/CatalogHiveIT.java
@@ -902,16 +902,20 @@ public void testAlterSchema() throws TException, InterruptedException {
         .alterSchema(
             ident,
             SchemaChange.removeProperty("key1"),
-            SchemaChange.setProperty("key2", "val2-alter"));
+            SchemaChange.setProperty("key2", "val2-alter"),
+            SchemaChange.updateComment("comment1"));
 
-    Map<String, String> properties2 = catalog.asSchemas().loadSchema(ident).properties();
+    Schema schema = catalog.asSchemas().loadSchema(ident);
+    Map<String, String> properties2 = schema.properties();
     Assertions.assertFalse(properties2.containsKey("key1"));
     Assertions.assertEquals("val2-alter", properties2.get("key2"));
+    Assertions.assertEquals("comment1", schema.comment());
 
     Database database = hiveClientPool.run(client -> client.getDatabase(schemaName));
     Map<String, String> properties3 = database.getParameters();
     Assertions.assertFalse(properties3.containsKey("key1"));
     Assertions.assertEquals("val2-alter", properties3.get("key2"));
+    Assertions.assertEquals("comment1", database.getDescription());
   }
 
   @Test

diff --git a/...com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java b/...com/datastrato/gravitino/integration/test/catalog/lakehouse/iceberg/CatalogIcebergIT.java
@@ -270,10 +270,12 @@ void testOperationIcebergSchema() {
     Assertions.assertTrue(schemaNames.contains(testSchemaName));
 
     // alert、load schema check.
-    schemas.alterSchema(schemaIdent, SchemaChange.setProperty("t1", "v1"));
+    schemas.alterSchema(
+        schemaIdent, SchemaChange.setProperty("t1", "v1"), SchemaChange.updateComment("comment1"));
     Schema schema = schemas.loadSchema(schemaIdent);
     String val = schema.properties().get("t1");
     Assertions.assertEquals("v1", val);
+    Assertions.assertEquals("comment1", schema.comment());
 
     Map<String, String> hiveCatalogProps =
         hiveCatalog.loadNamespaceMetadata(