-
Notifications
You must be signed in to change notification settings - Fork 152
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.
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
[590] Add Iceberg HMS Catalog Sync implementation #600
base: 590-CatalogSync
Are you sure you want to change the base?
[590] Add Iceberg HMS Catalog Sync implementation #600
Conversation
xtable-core/src/main/java/org/apache/xtable/catalog/hms/HMSCatalogSyncClient.java
Outdated
Show resolved
Hide resolved
xtable-core/src/main/java/org/apache/xtable/catalog/hms/IcebergHMSCatalogSyncHelper.java
Outdated
Show resolved
Hide resolved
a723014
to
ced6288
Compare
e50889b
to
c443f16
Compare
c443f16
to
96a9b13
Compare
xtable-core/src/main/java/org/apache/xtable/catalog/CatalogTableBuilder.java
Outdated
Show resolved
Hide resolved
893b1a6
to
eefbdd2
Compare
970c2ef
to
88c0471
Compare
<hudi.version>0.14.0</hudi.version> | ||
<aws.version>2.28.22</aws.version> | ||
<hive.version>2.3.9</hive.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a 4.X.X release, can we try using that or at least the 3.X.X line? I am worried that any vulnerability patches will not make it to the N-2 major version line.
34f18a0
to
25bf942
Compare
|
||
<!-- Iceberg dependencies --> | ||
<dependency> | ||
<groupId>org.apache.iceberg</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only used to get a class name as a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also used during HiveMetaHook#preCreateTable
as part of creating table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link me to where that is happening in this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not part of this branch, rather with storage_handler property set, metastore client executes the preCreateTable
method of the HiveMetaHook associated with the HiveIcebergStorageHandler during createTable operation
HMS client createTable: https://github.com/apache/hive/blob/rel/release-2.3.9/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java#L741
HiveMetahookLoader: https://github.com/apache/hive/blob/rel/release-2.3.9/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java#L3579
28959ae
to
4d0835b
Compare
xtable-hive-metastore/src/main/java/org/apache/xtable/hms/HMSCatalogTableBuilderFactory.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
public static Table newHmsTable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a unit test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has been tested as part of TestIcebergHMSCatalogTableBuilder#testGetCreateTableRequest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be in a test class for HMSCatalogTableBuilderFactory
Important Read
What is the purpose of the pull request
Add HMS catalog sync client implementation for Iceberg
Brief change log
Verify this pull request