[apache#3137] fix(spark-connector): change table comment though table…

… properties (apache#3490) ### What changes were proposed in this pull request? change table comment though table properties ### Why are the changes needed? Fix: apache#3137 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? add ut and IT
bknbkn · May 27, 2024 · 7dfa6a6 · 7dfa6a6
1 parent 048f114
commit 7dfa6a6
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 5 deletions.
diff --git a/...connector/src/main/java/com/datastrato/gravitino/spark/connector/catalog/BaseCatalog.java b/...connector/src/main/java/com/datastrato/gravitino/spark/connector/catalog/BaseCatalog.java
@@ -438,10 +438,17 @@ private String getDatabase(NameIdentifier gravitinoIdentifier) {
   static com.datastrato.gravitino.rel.TableChange transformTableChange(TableChange change) {
     if (change instanceof TableChange.SetProperty) {
       TableChange.SetProperty setProperty = (TableChange.SetProperty) change;
-      return com.datastrato.gravitino.rel.TableChange.setProperty(
-          setProperty.property(), setProperty.value());
+      if (ConnectorConstants.COMMENT.equals(setProperty.property())) {
+        return com.datastrato.gravitino.rel.TableChange.updateComment(setProperty.value());
+      } else {
+        return com.datastrato.gravitino.rel.TableChange.setProperty(
+            setProperty.property(), setProperty.value());
+      }
     } else if (change instanceof TableChange.RemoveProperty) {
       TableChange.RemoveProperty removeProperty = (TableChange.RemoveProperty) change;
+      Preconditions.checkArgument(
+          ConnectorConstants.COMMENT.equals(removeProperty.property()) == false,
+          "Gravitino doesn't support remove table comment yet");
       return com.datastrato.gravitino.rel.TableChange.removeProperty(removeProperty.property());
     } else if (change instanceof TableChange.AddColumn) {
       TableChange.AddColumn addColumn = (TableChange.AddColumn) change;

diff --git a/.../test/java/com/datastrato/gravitino/spark/connector/catalog/TestTransformTableChange.java b/.../test/java/com/datastrato/gravitino/spark/connector/catalog/TestTransformTableChange.java
@@ -5,7 +5,9 @@
 
 package com.datastrato.gravitino.spark.connector.catalog;
 
+import com.datastrato.gravitino.rel.TableChange.UpdateComment;
 import com.datastrato.gravitino.rel.expressions.literals.Literals;
+import com.datastrato.gravitino.spark.connector.ConnectorConstants;
 import org.apache.spark.sql.connector.catalog.ColumnDefaultValue;
 import org.apache.spark.sql.connector.catalog.TableChange;
 import org.apache.spark.sql.connector.expressions.LiteralValue;
@@ -40,13 +42,28 @@ void testTransformRemoveProperty() {
     Assertions.assertEquals("key", gravitinoRemoveProperty.getProperty());
   }
 
+  @Test
+  void testTransformUpdateComment() {
+    TableChange sparkSetProperty = TableChange.setProperty(ConnectorConstants.COMMENT, "a");
+    com.datastrato.gravitino.rel.TableChange tableChange =
+        BaseCatalog.transformTableChange(sparkSetProperty);
+    Assertions.assertTrue(
+        tableChange instanceof com.datastrato.gravitino.rel.TableChange.UpdateComment);
+    Assertions.assertEquals("a", ((UpdateComment) tableChange).getNewComment());
+
+    TableChange sparkRemoveProperty = TableChange.removeProperty(ConnectorConstants.COMMENT);
+    Assertions.assertThrowsExactly(
+        IllegalArgumentException.class,
+        () -> BaseCatalog.transformTableChange(sparkRemoveProperty));
+  }
+
   @Test
   void testTransformRenameColumn() {
-    String[] oldFiledsName = new String[] {"default_name"};
+    String[] oldFieldsName = new String[] {"default_name"};
     String newFiledName = "new_name";
 
     TableChange.RenameColumn sparkRenameColumn =
-        (TableChange.RenameColumn) TableChange.renameColumn(oldFiledsName, newFiledName);
+        (TableChange.RenameColumn) TableChange.renameColumn(oldFieldsName, newFiledName);
     com.datastrato.gravitino.rel.TableChange gravitinoChange =
         BaseCatalog.transformTableChange(sparkRenameColumn);
 
@@ -55,7 +72,7 @@ void testTransformRenameColumn() {
     com.datastrato.gravitino.rel.TableChange.RenameColumn gravitinoRenameColumn =
         (com.datastrato.gravitino.rel.TableChange.RenameColumn) gravitinoChange;
 
-    Assertions.assertArrayEquals(oldFiledsName, gravitinoRenameColumn.getFieldName());
+    Assertions.assertArrayEquals(oldFieldsName, gravitinoRenameColumn.getFieldName());
     Assertions.assertEquals(newFiledName, gravitinoRenameColumn.getNewName());
   }
 

diff --git a/...rc/test/java/com/datastrato/gravitino/spark/connector/integration/test/SparkCommonIT.java b/...rc/test/java/com/datastrato/gravitino/spark/connector/integration/test/SparkCommonIT.java
@@ -4,6 +4,7 @@
  */
 package com.datastrato.gravitino.spark.connector.integration.test;
 
+import com.datastrato.gravitino.spark.connector.ConnectorConstants;
 import com.datastrato.gravitino.spark.connector.integration.test.util.SparkTableInfo;
 import com.datastrato.gravitino.spark.connector.integration.test.util.SparkTableInfo.SparkColumnInfo;
 import com.datastrato.gravitino.spark.connector.integration.test.util.SparkTableInfoChecker;
@@ -387,6 +388,23 @@ void testAlterTableSetAndRemoveProperty() {
     Assertions.assertTrue(newProperties.containsKey("key2"));
   }
 
+  @Test
+  void testAlterTableUpdateComment() {
+    String tableName = "test_comment";
+    String comment = "comment1";
+    dropTableIfExists(tableName);
+
+    createSimpleTable(tableName);
+    sql(
+        String.format(
+            "ALTER TABLE %s SET TBLPROPERTIES('%s'='%s')",
+            tableName, ConnectorConstants.COMMENT, comment));
+    SparkTableInfo tableInfo = getTableInfo(tableName);
+    SparkTableInfoChecker checker =
+        SparkTableInfoChecker.create().withName(tableName).withComment(comment);
+    checker.check(tableInfo);
+  }
+
   @Test
   void testAlterTableAddAndDeleteColumn() {
     String tableName = "test_column";