[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.

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

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?

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

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

steveburnett · 2025-01-06T15:47:02Z

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

Looks good, thanks!

steveburnett · 2025-02-11T14:40:17Z

New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR.


:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

hantangwangd · 2025-02-11T15:08:42Z

@steveburnett Thanks for the reminder, fixed!

hantangwangd · 2025-02-11T15:10:49Z

This is simply rebased to the newest commit list, please take a look when available, thanks a lot! @ZacBlanco @agrawalreetika @tdcmeehan

ZacBlanco

just some minor things

...eberg/src/test/java/com/facebook/presto/iceberg/hive/TestRenameTableOnFragileFileSystem.java

ZacBlanco · 2025-02-12T17:14:54Z

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

+                            metadataFileSystem.delete(newTablePermissionDir, true);
+                        }
+                        catch (IOException e) {
+                            // ignore


maybe log a warning here with the error?

Sure, done!

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

ZacBlanco · 2025-02-12T17:20:03Z

...eberg/src/test/java/com/facebook/presto/iceberg/hive/TestRenameTableOnFragileFileSystem.java

+        @Override
+        public boolean mkdirs(Path f, FsPermission permission) throws IOException
+        {
+            if (this.failSignal.get() == FailSignal.COPY) {


Why is the signal for mkdirs COPY? Maybe just rename to MKDIRS?

The mkdirs operation is a step of the COPY operation, we want to simulate the behavior of failure during the COPY process. But I think it's fine to use MKDIRS to reduce the confusion, fixed!

agrawalreetika · 2025-02-12T17:49:17Z

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

+            if (!metadataFileSystem.rename(originalMetadataDirectory, newMetadataDirectory)) {
+                throw new PrestoException(HIVE_METASTORE_ERROR,
+                        format("Could not rename table, because of fail to rename directory %s to %s", originalMetadataDirectory, newMetadataDirectory));


Should we extract this rename in a utility method and use that instead here and above?

agrawalreetika · 2025-02-12T18:11:18Z

...eberg/src/test/java/com/facebook/presto/iceberg/hive/TestRenameTableOnFragileFileSystem.java

+            createFile(fileSystem, new Path(originSchemaMetadataPath), DATABASE_CODEC.toBytes(databaseMetadata));
+            createFile(fileSystem, new Path(newSchemaMetadataPath), DATABASE_CODEC.toBytes(databaseMetadata));
+            createFile(fileSystem, new Path(originTableMetadataPath), TABLE_CODEC.toBytes(tableMetadata));
+            createFile(fileSystem, new Path(originTablePermissionFilePath), new byte[128]);
+


Let's extract this common code to a method use that instead in all the places where these file setup is done to reduce repetition

agrawalreetika · 2025-02-12T18:12:40Z

...eberg/src/test/java/com/facebook/presto/iceberg/hive/TestRenameTableOnFragileFileSystem.java

+            assertFalse(fileSystem.exists(new Path(newTableMetadataPath)));
+            assertFalse(fileSystem.exists(new Path(newTablePermissionDirPath)));
+            assertFalse(fileSystem.exists(new Path(newTablePermissionFilePath)));
+            assertTrue(fileSystem.exists(new Path(originTableMetadataPath)));
+            assertTrue(fileSystem.exists(new Path(originTablePermissionDirPath)));
+            assertTrue(fileSystem.exists(new Path(originTablePermissionFilePath)));


Extract this common code in a method?

agrawalreetika · 2025-02-12T18:16:52Z

...eberg/src/test/java/com/facebook/presto/iceberg/hive/TestRenameTableOnFragileFileSystem.java

+    }
+
+    @Test
+    public void testRenameTableFailCausedByRenameTableSchemaFile()


I think we can reduce the redundancy in different test methods by creating a new method with a common code in it and send different FailSignal as a parameter for different test cases?

Good suggestion. I have refactor the code, extracted the common code logic into a base method testRenameTableWithFailSignalAndValidation. It accepts fail signal, customized rename logic and verification logic as parameters, and be invoked by all the test methods.

hantangwangd · 2025-02-13T01:43:21Z

Hi @ZacBlanco @agrawalreetika, the comments have all been addressed, please take a look when convenient, thanks!

agrawalreetika

LGTM

ZacBlanco

LGTM!

hantangwangd · 2025-02-13T07:51:44Z

Hi @jaystarshot, can you please help take a final look at this PR when available? Thanks a lot!

again

jaystarshot · 2025-02-13T17:59:38Z

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

+    }
+
+    private void renamePath(Path originalPath, Path targetPath, String errorMessage)
+            throws IOException


Previously this code path seems to throwing a PrestoException in some places with a HIVE_METASTORE_ERROR

This is an extracted private method, the IOException thrown by it will always be caught by other methods and converted to a PrestoException with HIVEMETASTORE_ERROR as you mentioned.

jaystarshot · 2025-02-13T18:00:18Z

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

+                metadataFileSystem.delete(new Path(originalMetadataDirectory, PRESTO_PERMISSIONS_DIRECTORY_NAME), true);
+            }
+            catch (IOException e) {
+                // ignore


Exception seems to be ignored here?

This is an intentional exception ignorance, as the deletion action here will not affect the entire rename behavior.

jaystarshot

Sorry I am not well versed with how the iceberg connector works but should there be iceberg specific code in the presto hive metastore

jaystarshot · 2025-02-13T18:00:39Z

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

+                            metadataFileSystem.delete(newTablePermissionDir, true);
+                        }
+                        catch (IOException e) {
+                            LOG.warn("Could not delete table permission directory: %s", newTablePermissionDir);


Same here, should we ignore exception here?

The same as above.

hantangwangd · 2025-02-13T19:03:15Z

Sorry I am not well versed with how the iceberg connector works but should there be iceberg specific code in the presto hive metastore

@jaystarshot Thanks for your review. This is a good question. At present, all lake houses share the same HMS code, so that some Iceberg related code logic already existed in FileHiveMetastore before this change. But @ZacBlanco and I has discussed about this in the above conversation, see here. We will extract a subclass of FileHiveMetastore for Iceberg in a later PR.

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 a team and ZacBlanco as code owners January 1, 2025 17:21

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

agrawalreetika reviewed Jan 3, 2025

View reviewed changes

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java Outdated Show resolved Hide resolved

agrawalreetika reviewed Jan 3, 2025

View reviewed changes

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java Outdated Show resolved Hide resolved

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

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

ZacBlanco previously approved these changes Jan 8, 2025

View reviewed changes

agrawalreetika previously approved these changes Jan 8, 2025

View reviewed changes

tdcmeehan self-assigned this Jan 14, 2025

hantangwangd dismissed stale reviews from agrawalreetika and ZacBlanco via 8bfd938 February 11, 2025 03:30

hantangwangd force-pushed the support_hive_catalog_rename_table branch from 7330926 to 8bfd938 Compare February 11, 2025 03:30

ZacBlanco requested changes Feb 12, 2025

View reviewed changes

agrawalreetika requested changes Feb 12, 2025

View reviewed changes

hantangwangd force-pushed the support_hive_catalog_rename_table branch 2 times, most recently from 3110ae5 to 5c30649 Compare February 13, 2025 01:21

[Iceberg]Support renaming table on hive file catalog

cf83835

hantangwangd force-pushed the support_hive_catalog_rename_table branch from 5c30649 to cf83835 Compare February 13, 2025 01:36

agrawalreetika approved these changes Feb 13, 2025

View reviewed changes

ZacBlanco approved these changes Feb 13, 2025

View reviewed changes

hantangwangd requested a review from jaystarshot February 13, 2025 07:50

jaystarshot previously approved these changes Feb 13, 2025

View reviewed changes

jaystarshot reviewed Feb 13, 2025

View reviewed changes

jaystarshot approved these changes Feb 15, 2025

View reviewed changes

hantangwangd merged commit db92976 into prestodb:master Feb 15, 2025
54 checks passed

hantangwangd deleted the support_hive_catalog_rename_table branch February 15, 2025 03:49

hantangwangd mentioned this pull request Feb 16, 2025

Subclass FileHiveMetastore for Iceberg connector #24573

Merged

6 tasks

This was referenced Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

Add release notes for 0.292 unix280/presto#6

Closed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

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

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

Uh oh!

Conversation

hantangwangd commented Jan 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Jan 3, 2025

Uh oh!

hantangwangd commented Jan 4, 2025

Uh oh!

agrawalreetika left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hantangwangd commented Jan 6, 2025

Uh oh!

steveburnett commented Jan 6, 2025

Uh oh!

steveburnett commented Feb 11, 2025

Uh oh!

hantangwangd commented Feb 11, 2025

Uh oh!

hantangwangd commented Feb 11, 2025

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hantangwangd commented Feb 13, 2025

Uh oh!

agrawalreetika left a comment

Choose a reason for hiding this comment

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

hantangwangd commented Feb 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hantangwangd commented Jan 1, 2025 •

edited

Loading