[Iceberg]Support renaming table on hive file catalog #24312

hantangwangd · 2025-01-01T09:42:47Z

Description

This PR support renaming table behavior for iceberg tables on hive file catalog.

Motivation and Context

Support renaming table for Iceberg connector on as many catalog types as possible

Impact

Iceberg connector configured with hive file catalogs can now support renaming table

Test Plan

Enable existing test cases for renaming table on HIVE file catalog

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support of ``renaming table`` for Iceberg connector when configured with ``HIVE`` file catalog. :pr:`24312`

agrawalreetika · 2025-01-03T05:30:00Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

        try {
-            if (!metadataFileSystem.rename(getTableMetadataDirectory(databaseName, tableName), getTableMetadataDirectory(newDatabaseName, newTableName))) {
-                throw new PrestoException(HIVE_METASTORE_ERROR, "Could not rename table directory");
+            if (!isIcebergTable(table)) {


Nit - How about we put the positive condition in if-else instead since the implementation is valid for tables and views both :

if (isIcebergTable) {} else {}

agrawalreetika · 2025-01-03T05:30:40Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

+            }
+            else {
+                // If the directory `.prestoPermissions` exists, copy it to the new table metadata directory
+                if (metadataFileSystem.exists(new Path(metadataDirectory, PRESTO_PERMISSIONS_DIRECTORY_NAME))) {


Also I think it would be better to move Iceberg logic handle in different method and call that from here.

Good suggestion, I have refactored the code, extracted two methods to make it more concise. Please take a look when available.

ZacBlanco

I think the changes are fine, however the code complexity seems high due to all the nested conditionals. I would consider factoring out some methods or subclassing as mentioned in my comment.

ZacBlanco · 2025-01-03T16:44:53Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

        try {
-            if (!metadataFileSystem.rename(getTableMetadataDirectory(databaseName, tableName), getTableMetadataDirectory(newDatabaseName, newTableName))) {
-                throw new PrestoException(HIVE_METASTORE_ERROR, "Could not rename table directory");
+            if (!isIcebergTable(table)) {


I don't think we need to do it for this PR, but all these conditionals make me think that it is worth considering to subclass the FileHiveMetastore and create a FileIcebergHiveMetastore or something similar to capture the differences in behavior. It seems a bit messy currently.

Agreed, seems there are indeed some implementation details that differ between them and need to be special handled. It is worth considering to put these different details into Iceberg's own subclass.

steveburnett · 2025-01-03T17:09:36Z

Thanks for the release note! Nit of formatting suggestion:

== RELEASE NOTES ==

Iceberg Connector Changes
* Add support of ``renaming table`` for Iceberg connector when configured with ``HIVE`` file catalog. :pr:`24312`

hantangwangd · 2025-01-04T03:21:22Z

Thanks @steveburnett, fixed. Please take a look when available.

agrawalreetika

@hantangwangd Can we add some tests in iceberg which tests these newly added code lines?

agrawalreetika · 2025-01-04T06:04:54Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

+    {
+        try {
+            if (!metadataFileSystem.rename(originalMetadataDirectory, newMetadataDirectory)) {
+                throw new PrestoException(HIVE_METASTORE_ERROR, "Could not rename table directory");


Nit: We can add some more details in exception here around which table directory is being renamed

agrawalreetika · 2025-01-04T06:05:05Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

+
+            // Rename file `.prestoSchema` to change it to the new metadata path
+            // This will atomically execute the table renaming behavior
+            if (!metadataFileSystem.rename(new Path(originalMetadataDirectory, PRESTO_SCHEMA_FILE_NAME), new Path(newMetadataDirectory, PRESTO_SCHEMA_FILE_NAME))) {
                throw new PrestoException(HIVE_METASTORE_ERROR, "Could not rename table directory");


Nit: We can add some more details in exception here around which table directory is being renamed

agrawalreetika · 2025-01-04T06:05:12Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

+            if (metadataFileSystem.exists(new Path(originalMetadataDirectory, PRESTO_PERMISSIONS_DIRECTORY_NAME))) {
+                if (!FileUtil.copy(metadataFileSystem, new Path(originalMetadataDirectory, PRESTO_PERMISSIONS_DIRECTORY_NAME),
+                        metadataFileSystem, new Path(newMetadataDirectory, PRESTO_PERMISSIONS_DIRECTORY_NAME), false, metadataFileSystem.getConf())) {
+                    throw new PrestoException(HIVE_METASTORE_ERROR, "Could not rename table directory");


Nit: We can add some more details in exception here around which table directory is being renamed

hantangwangd · 2025-01-06T07:40:07Z

Can we add some tests in iceberg which tests these newly added code lines?

I constructed a test environment based on a customized ExtendedFileSystem which can simulate the scenarios where certain file operations fail. In this way, I added some test cases to test these newly added code branches. Please take a look when available. And open to any better test solutions. @agrawalreetika @ZacBlanco

hantangwangd force-pushed the support_hive_catalog_rename_table branch 2 times, most recently from 7cd47c9 to 366408c Compare January 1, 2025 12:02

hantangwangd marked this pull request as ready for review January 1, 2025 17:21

hantangwangd requested review from ZacBlanco and a team as code owners January 1, 2025 17:21

hantangwangd requested review from presto-oss, agrawalreetika and tdcmeehan and removed request for presto-oss January 1, 2025 17:21

agrawalreetika reviewed Jan 3, 2025

View reviewed changes

ZacBlanco previously approved these changes Jan 3, 2025

View reviewed changes

hantangwangd dismissed ZacBlanco’s stale review via 4a0b64d January 4, 2025 03:00

hantangwangd force-pushed the support_hive_catalog_rename_table branch from 366408c to 4a0b64d Compare January 4, 2025 03:00

agrawalreetika reviewed Jan 4, 2025

View reviewed changes

hantangwangd force-pushed the support_hive_catalog_rename_table branch from 4a0b64d to e3fa5dc Compare January 5, 2025 17:19

hantangwangd marked this pull request as draft January 5, 2025 17:21

[Iceberg]Support renaming table on hive file catalog

7330926

hantangwangd force-pushed the support_hive_catalog_rename_table branch from e3fa5dc to 7330926 Compare January 6, 2025 03:44

hantangwangd marked this pull request as ready for review January 6, 2025 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Iceberg]Support renaming table on hive file catalog #24312

[Iceberg]Support renaming table on hive file catalog #24312

hantangwangd commented Jan 1, 2025 •

edited

Loading

agrawalreetika Jan 3, 2025

hantangwangd Jan 4, 2025

agrawalreetika Jan 3, 2025

hantangwangd Jan 4, 2025

ZacBlanco left a comment

ZacBlanco Jan 3, 2025

hantangwangd Jan 4, 2025

steveburnett commented Jan 3, 2025

hantangwangd commented Jan 4, 2025

agrawalreetika left a comment

agrawalreetika Jan 4, 2025

hantangwangd Jan 6, 2025

agrawalreetika Jan 4, 2025

agrawalreetika Jan 4, 2025

hantangwangd commented Jan 6, 2025

[Iceberg]Support renaming table on hive file catalog #24312

Are you sure you want to change the base?

[Iceberg]Support renaming table on hive file catalog #24312

Conversation

hantangwangd commented Jan 1, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Jan 3, 2025

hantangwangd commented Jan 4, 2025

agrawalreetika left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantangwangd commented Jan 6, 2025

hantangwangd commented Jan 1, 2025 •

edited

Loading