feat(datafusion): Support CREATE EXTERNAL TABLE with catalog #2022
Conversation
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, just finished first round of review, it looks great!
| let table_ident = TableIdent::new(namespace.clone(), name.clone()); | ||
|
|
||
| // Check if table exists before attempting to load | ||
| if !catalog |
There was a problem hiding this comment.
When datafusion calls this method to create a table, it already has deregistered table. But since datafusion allows no-iceberg catalog to register iceberg table, this check may still miss match. I think we should do is that:
if catalog.table.exists(...) {
return Error::new(ErrorKind::DataInvalid, "Table already exists in iceberg catalog ..., this maybe a misconfig datafusion catalog provider vs iceberg catalog.");
}
There was a problem hiding this comment.
Do you mean that we should check if the table already exists in the IcebergCatalogProvider/SchemaProvider? I think that verification is done when datafusion is calling register_table after TableProviderFactory::create: https://github.com/apache/datafusion/blob/a9e6d4be4a63c2180814a627d3b45cd0adf5b61c/datafusion/core/src/execution/context/mod.rs#L803
I don't think we have a way to verify that within TableProviderFactory
There was a problem hiding this comment.
No, I mean when we hit this code path, we should actually create a new table rather than loading an existing one.
|
|
||
| # Create external table with bare name (uses "default" namespace) | ||
| statement ok | ||
| CREATE EXTERNAL TABLE ns_table STORED AS ICEBERG LOCATION '' |
There was a problem hiding this comment.
Can we have a test for partitioned table?
| let table_ident = TableIdent::new(namespace.clone(), name.clone()); | ||
|
|
||
| // Check if table exists before attempting to load | ||
| if !catalog |
There was a problem hiding this comment.
No, I mean when we hit this code path, we should actually create a new table rather than loading an existing one.
| catalog.create_namespace(&namespace, HashMap::new()).await?; | ||
|
|
||
| // Create partitioned test table (unpartitioned tables are now created via SQL) | ||
| Self::create_partitioned_table(&catalog, &namespace).await?; |
There was a problem hiding this comment.
We should remove this one.
Which issue does this PR close?
CREATE EXTERNAL TABLEbacked by a Catalog with DataFusion #2021What changes are included in this PR?
CREATE EXTERNAL TABLEwith catalog so the table is writableAre these changes tested?
Added some uts and sqllogictests