coolderli commented on code in PR #4097:
URL: https://github.com/apache/gravitino/pull/4097#discussion_r1687479979


##########
flink-connector/src/main/java/org/apache/gravitino/flink/connector/catalog/BaseCatalog.java:
##########
@@ -280,10 +283,72 @@ public void createTable(ObjectPath tablePath, 
CatalogBaseTable table, boolean ig
     }
   }
 
+  /**
+   * The method only is used to change the properties and comments. To alter 
columns, use the other
+   * alterTable API and provide a list of TableChange's.
+   *
+   * @param tablePath path of the table or view to be modified
+   * @param newTable the new table definition
+   * @param ignoreIfNotExists flag to specify behavior when the table or view 
does not exist: if set
+   *     to false, throw an exception, if set to true, do nothing.
+   * @throws TableNotExistException if the table not exists.
+   * @throws CatalogException in case of any runtime exception.
+   */
   @Override
-  public void alterTable(ObjectPath objectPath, CatalogBaseTable 
catalogBaseTable, boolean b)
+  public void alterTable(ObjectPath tablePath, CatalogBaseTable newTable, 
boolean ignoreIfNotExists)

Review Comment:
   The `alterTable(ObjectPath tablePath, CatalogBaseTable newTable, boolean 
ignoreIfNotExists)` needs to get the TableChange manually. I think we should 
`alterTable(
         ObjectPath tablePath,
         CatalogBaseTable newTable,
         List<org.apache.flink.table.catalog.TableChange> tableChanges,
         boolean ignoreIfNotExists)` to set properties to provided removing the 
gravitino properties.
   
   I refer the implementation in iceberg: 
https://github.com/apache/iceberg/blob/main/flink/v1.17/flink/src/main/java/org/apache/iceberg/flink/FlinkCatalog.java#L472



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to