[GitHub] [hudi] jinxing64 commented on a diff in pull request #5320: [HUDI-3861] update tblp 'path' when rename table

[GitBox <gi...@apache.org>]

jinxing64 commented on code in PR #5320:
URL: https://github.com/apache/hudi/pull/5320#discussion_r883469001


##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterHoodieTableRenameCommand.scala:
##########
@@ -46,6 +45,15 @@ class AlterHoodieTableRenameCommand(
 
       // Call AlterTableRenameCommand#run to rename table in meta.
       super.run(sparkSession)
+
+      // update table properties path in every op
+      if (hoodieCatalogTable.catalogProperties.contains("path")) {
+        val catalogTable = sparkSession.sessionState.catalog.getTableMetadata(newName)
+        val path = catalogTable.storage.locationUri.get.getPath
+        logInfo(s"alter ${oldName} name to ${newName}, update tblp 'path' to ${path}")

Review Comment:
   @KnightChess 
   You thumbed up on this comment. Would you please update the log accordingly ?
   From my side, it's also fine to just remove it -- Renaming a table should update the path property by default, it's ok no logging. But it depends on you :)



##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/TestAlterTable.scala:
##########
@@ -169,4 +168,77 @@ class TestAlterTable extends HoodieSparkSqlTestBase {
       }
     }
   }
+
+  test("Test Alter Rename Table") {
+    withTempDir { tmp =>
+      Seq("cow", "mor").foreach { tableType =>

Review Comment:
   Many of the current sql tests includes both COW and MOR scenarios. But this PR targeting to resolve the issue of RENAME doesn't bother much with whether it's COW or MOR.
   Shall we just test COW, which I believe should be enough?



##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/TestAlterTable.scala:
##########
@@ -169,4 +168,77 @@ class TestAlterTable extends HoodieSparkSqlTestBase {
       }
     }
   }
+

Review Comment:
   Let's give a better testing name, how about "Test renaming MANAGED and EXTERNAL tables"



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org