[AMORO-2811][Bug][FLINK]: flink unified catalog doesn't support mixed_hive #2814
Conversation
github-actions
bot
added
module:mixed-flink
|
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.
This method is used to create a table. How to load the table before creating it?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
| 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.
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