Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Subclass FileHiveMetastore for Iceberg connector #24573

Merged
merged 1 commit into from
Feb 19, 2025
Merged

Subclass FileHiveMetastore for Iceberg connector #24573

merged 1 commit into from
Feb 19, 2025

Conversation

hantangwangd
Copy link
Member

@hantangwangd hantangwangd commented Feb 16, 2025 •
edited
Loading

Description

Currently, Iceberg connector and Hive connector use the same file based HMS code, however, there are some implementation details that differ between them and need to be special handled. This PR create a dedicated subclass of FileHiveMetastore for Iceberg connector, and move the Iceberg specific logic from FileHiveMetastore into this dedicated subclass. As discussed in here.

Motivation and Context

The modifications of Iceberg specific logic on hive catalog will not affect the FileHiveMetastore in presto-hive-common.

Impact

N/A

Test Plan

  • Make sure the refactor do not affect all existing tests for Hive and Iceberg.

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 a dedicated subclass of `FileHiveMetastore` for Iceberg connector to capture and isolate the differences in behavior.

@hantangwangd hantangwangd force-pushed the subclass_file_hive_metastore branch from bd247c7 to 7fd5ce8 Compare February 16, 2025 15:31
@hantangwangd hantangwangd marked this pull request as ready for review February 16, 2025 16:36
@hantangwangd hantangwangd requested review from ZacBlanco and a team as code owners February 16, 2025 16:36
ZacBlanco
ZacBlanco reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two initial things:

presto-iceberg/src/main/java/com/facebook/presto/iceberg/hive/FileIcebergHiveMetastore.java Outdated

@Override
protected void validateReplaceTableType(Table originTable, Table newTable)
{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this override? From a cursory glance it seems we should still check if a table is a virtual view?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we need this override. The logic of the code is that, in Hive or other non-Iceberg lake houses, only views can be updated with replaceTable; but in Iceberg, views and tables all can be updated with replaceTable. Moreover, Iceberg need this replaceTable to support its transaction commit.

yingsu00
yingsu00 reviewed Feb 18, 2025
View reviewed changes
...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java Outdated
@@ -507,78 +506,33 @@ public synchronized MetastoreOperationResult replaceTable(MetastoreContext metas
return EMPTY_RESULT;
}

protected void validateReplaceTableType(Table originTable, Table newTable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the protected functions to be below the public ones and in the order of being called

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for the rest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have adjusted the position of fields and methods in the whole class according to your suggestion, please take a look when convenient, thanks.

@hantangwangd hantangwangd force-pushed the subclass_file_hive_metastore branch from 7fd5ce8 to d86e7fe Compare February 18, 2025 15:00
@yingsu00 yingsu00 merged commit 4a0bbc9 into prestodb:master Feb 19, 2025
53 checks passed
This was referenced Mar 10, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacBlanco ZacBlanco ZacBlanco approved these changes

@yingsu00 yingsu00 yingsu00 approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@imjalpreet imjalpreet Awaiting requested review from imjalpreet

@agrawalreetika agrawalreetika Awaiting requested review from agrawalreetika

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hantangwangd @ZacBlanco @yingsu00