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
[AMORO-2811][Bug][FLINK]: flink unified catalog doesn't support mixed_hive #2814
base: master
Are you sure you want to change the base?
Conversation
cc @YesOrNo828 @xieyi888 PTAL. |
if (!configuration.contains(TABLE_FORMAT)) { | ||
// if user doesn't specify the table format, we try to load the table and get the table | ||
// format. | ||
try { | ||
AmoroTable amoroTable = | ||
unifiedCatalog.loadTable(tableIdentifier.getDatabase(), tableIdentifier.getTableName()); | ||
format = amoroTable.format(); | ||
} catch (Throwable t) { | ||
LOG.warn( | ||
"We can't load table {}.{} through unifiedCatalog, use default format {}", | ||
tablePath.getDatabaseName(), | ||
tablePath.getObjectName(), | ||
format.name()); | ||
} | ||
} | ||
final TableFormat catalogFormat = format; |
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 method is used to create a table. How to load the table before creating it?
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 method is used to create a table. How to load the table before creating it?
public void createTable(ObjectPath tablePath, CatalogBaseTable table, boolean ignoreIfExists)
In the Flink SQL scenario, when users are writing SQL, this situation arises when they use "create table if not exists" to define a table that already exists. The parameter "ignoreIfExists" is used to ignore the existence of the table.
AmoroTable amoroTable = | ||
unifiedCatalog.loadTable(tableIdentifier.getDatabase(), tableIdentifier.getTableName()); | ||
format = amoroTable.format(); | ||
} catch (Throwable t) { |
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
catch (org.apache.amoro.NoSuchTableException e)
would be better
if (!configuration.contains(TABLE_FORMAT)) { | ||
// if user doesn't specify the table format, we try to load the table and get the table | ||
// format. | ||
try { | ||
AmoroTable amoroTable = | ||
unifiedCatalog.loadTable(tableIdentifier.getDatabase(), tableIdentifier.getTableName()); | ||
format = amoroTable.format(); | ||
} catch (Throwable t) { | ||
LOG.warn( | ||
"We can't load table {}.{} through unifiedCatalog, use default format {}", | ||
tablePath.getDatabaseName(), | ||
tablePath.getObjectName(), | ||
format.name()); | ||
} | ||
} | ||
final TableFormat catalogFormat = format; | ||
AbstractCatalog catalog = | ||
getOriginalCatalog(format).orElseGet(() -> createOriginalCatalog(tableIdentifier, format)); | ||
getOriginalCatalog(format) | ||
.orElseGet(() -> createOriginalCatalog(tableIdentifier, catalogFormat)); |
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.
If table exists, we load table and get tableFormat for it, Otherwise, we use default tableFormat to create table, which is MIXED_ICEBERG.
Did we should force user to add 'table.format' option in create table statement here?
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.
Did we should force user to add 'table.format' option in create table statement?
Good idea. If the table cannot be loaded via unifiedCatalog and no format is specified, we should report an error to prompt the user to specify the format. WDYT?
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 we could support a property of catalog which define a defult table format when creating a table without specific the format.
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 we could support a property of catalog which define a defult table format when creating a table without specific the format.
IMO, Configuring the format on both the table and the catalog at the same time can easily mislead users. The "Unified Catalog" is designed to allow users not to worry about the underlying format (except when it is created).
Let's listen to some other opinions. WDYT @YesOrNo828 @xieyi888
@@ -221,8 +226,25 @@ public void createTable(ObjectPath tablePath, CatalogBaseTable table, boolean ig | |||
TableIdentifier tableIdentifier = | |||
TableIdentifier.of( | |||
unifiedCatalog.name(), tablePath.getDatabaseName(), tablePath.getObjectName()); | |||
if (!configuration.contains(TABLE_FORMAT)) { |
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.
if (!configuration.contains(TABLE_FORMAT)) { | |
if (!configuration.contains(TABLE_FORMAT) && ignoreIfExists && unfiiedCatalog.tableExists()) { |
if (!configuration.contains(TABLE_FORMAT)) { | ||
// if user doesn't specify the table format, we try to load the table and get the table | ||
// format. | ||
try { | ||
AmoroTable amoroTable = | ||
unifiedCatalog.loadTable(tableIdentifier.getDatabase(), tableIdentifier.getTableName()); | ||
format = amoroTable.format(); | ||
} catch (Throwable t) { | ||
LOG.warn( | ||
"We can't load table {}.{} through unifiedCatalog, use default format {}", | ||
tablePath.getDatabaseName(), | ||
tablePath.getObjectName(), | ||
format.name()); | ||
} | ||
} | ||
final TableFormat catalogFormat = format; | ||
AbstractCatalog catalog = | ||
getOriginalCatalog(format).orElseGet(() -> createOriginalCatalog(tableIdentifier, format)); | ||
getOriginalCatalog(format) | ||
.orElseGet(() -> createOriginalCatalog(tableIdentifier, catalogFormat)); |
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 we could support a property of catalog which define a defult table format when creating a table without specific the format.
Why are the changes needed?
Close #2811 .
Brief change log
A unifiedCatalog which metastore is hms.
when create a mixed-hive table:
when create a iceberg table
when create a mixed-iceberg table
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation