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
[FLINK-12232][hive] Create HiveCatalog and support database related operations in HiveCatalog #8337
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
PR looks good. I left some comments for consideration. Thanks.
// HDFS path of the database | ||
private String location; | ||
// Comment of the database | ||
private String comment = "This is a generic catalog database."; |
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.
This shouldn't be a "generic" catalog, right?
public static Database createHiveDatabase(String dbName, CatalogDatabase db) { | ||
HiveCatalogDatabase hiveCatalogDatabase = (HiveCatalogDatabase) db; | ||
|
||
return new Database( |
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.
Just curious, is there any use for "comment"?
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.
I got confused between 'description' and 'comment'. I've created #8340 to complete the javadoc for description. Will address "comment"
// Comment of the database | ||
private String comment = "This is a generic catalog database."; | ||
|
||
public HiveCatalogDatabase(Map<String, String> properties) { |
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.
In other meta classes, properties are optional, where here it's required. Could we make it consistent? If all three variables are optional, we should have a default constructor.
throws DatabaseNotExistException, CatalogException { | ||
Database hiveDb; | ||
|
||
try { |
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.
Maybe we should extract this part to the base class, as the logic is the same for generic and hive.
public void createDatabase(String name, CatalogDatabase database, boolean ignoreIfExists) | ||
throws DatabaseAlreadyExistException, CatalogException { | ||
try { | ||
client.createDatabase(HiveCatalogUtil.createHiveDatabase(name, database)); |
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.
I think the logics related to hive metastore client can be shared. For example, createDatabase() only needs a hive database object, which might be created differently in the two catalogs. However, once such a object is created, the remaining logic is the same and can be shared.
public void alterDatabase(String name, CatalogDatabase newDatabase, boolean ignoreIfNotExists) throws DatabaseNotExistException, CatalogException { | ||
try { | ||
if (databaseExists(name)) { | ||
client.alterDatabase(name, HiveCatalogUtil.createHiveDatabase(name, newDatabase)); |
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.
Same as above.
@flinkbot attention @KurtYoung |
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.
LGTM, +1 to merge
Merged |
What is the purpose of the change
This PR creates HiveCatalog in flink-connector-hive module and implements database related APIs for HiveCatalog.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation
Documentation will be added later