Repository navigation
Add an extension point to allow SQL based registration of external catalogs - #25112
Conversation
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
Moved back to draft; still some work to do based on the CI results. |
|
@geoffreyclaude @Jefffrey in #19383 an |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25112 +/- ##
==========================================
- Coverage 82.51% 82.49% -0.02%
==========================================
Files 1140 1140
Lines 438568 439019 +451
Branches 438568 439019 +451
==========================================
+ Hits 361881 362171 +290
- Misses 54832 54962 +130
- Partials 21855 21886 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
For now I've adapted the example to use |
@pepijnve That's a very reasonable adaptation of the example I think! It keeps the custom sql parser which is the core of the example. Of course it's way less useful as a template since you add native |
|
I plan to review this today |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @pepijnve -- I think this looks good to go to me
I left a bunch of comments. The ones I think are needed before merging:
- Remove the submodule pins
- add the
cascademethod to thederegister_catalogtrait method and relevant tests.
Thank you for this -- I think it is a great extension point and the PR was easy to read and well tested
| //! Note: DataFusion supports `CREATE EXTERNAL CATALOG` out-of-the-box making use of | ||
| //! [`CatalogProviderFactory`](datafusion::catalog::CatalogProviderFactory). This example | ||
| //! is a partial reimplementation of the existing functionality to demonstrate how to extend the | ||
| //! SQL parser. |
There was a problem hiding this comment.
I found this somewhat confusing -- the comments are referring to CREATE EXTERNAL CATALOG but the example seems to use CREATE FOREIGN CATALOG 🤔
I think the point is that this example is showing how to support CREATE FOREIGN CATALOG which is similar in functionality to the (about to be) built in feature of CREATE EXTERNAL CATALOG)
| //! Note: DataFusion supports `CREATE EXTERNAL CATALOG` out-of-the-box making use of | |
| //! [`CatalogProviderFactory`](datafusion::catalog::CatalogProviderFactory). This example | |
| //! is a partial reimplementation of the existing functionality to demonstrate how to extend the | |
| //! SQL parser. | |
| //! Note: DataFusion supports `CREATE EXTERNAL CATALOG` out-of-the-box making use of | |
| //! [`CatalogProviderFactory`](datafusion::catalog::CatalogProviderFactory). This example | |
| //! implements very similar functionality to demonstrate how to extend the SQL parser. |
There was a problem hiding this comment.
TBH I wasn't sure how to describe this well. The point I was trying to get across is that this example is only a partial reimplementation of the functionality provided by the built-in 'external' support. There's no delegation to the registered factories in handle_create_foreign_catalog, it's a simple hardcoded example only.
I think it would actually be better to replace this example with something else that doesn't overlap with built-in functionality, but I took the easy way out for myself mainly due to lack of inspiration for a different (but still simple to implement) custom syntax alternative.
|
|
||
| #[tokio::test] | ||
| async fn create_external_catalog_with_factory() -> Result<()> { | ||
| let mut state = SessionStateBuilder::new().with_default_features().build(); |
There was a problem hiding this comment.
this is consistent with the other code in this test, so it is all good. However, it seems like a lot of ceremony. It would be really nice, perhaps a follow on PR, to use the SessionStateBuilder for this. Something like
let ctx: SessionContext = SessionStateBuilder::new()
.with_default_features()
.with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
.build()
.into();There was a problem hiding this comment.
I've gone ahead and done this already, and while I was at it changed SessiontStateBuilder::with_table_factory to accept impl Into<String> as well.
|
|
||
| #[test] | ||
| fn drop_catalog() -> Result<(), DataFusionError> { | ||
| let sql = "DROP CATALOG c"; |
There was a problem hiding this comment.
can you also please add a test for DROP CATALOG c CASCADE ?
| "Catalog should have been dropped!" | ||
| ); | ||
|
|
||
| // dropping again should fail without IF EXISTS |
There was a problem hiding this comment.
can you also please add a test for DROP CATALOG cat CASCADE ?
I would expect it to error with not supported / not implemented
| before a query ever runs. Sometimes it is useful to let users attach a | ||
| catalog dynamically from SQL instead — for example, a catalog backed by a | ||
| remote catalog service such as an Iceberg REST catalog. This is exactly | ||
| analogous to how [`TableProviderFactory`] lets `CREATE EXTERNAL TABLE` |
| }), | ||
| )), | ||
| _ => not_impl_err!( | ||
| "Only `DROP TABLE/VIEW/SCHEMA ...` statement is supported currently" |
There was a problem hiding this comment.
we should probably update this message to also mention catalog
| DdlStatement::DropCatalog(datafusion_expr::DropCatalog { | ||
| name: object_name_to_string(&name), | ||
| if_exists, | ||
| schema: DFSchemaRef::new(DFSchema::empty()), |
There was a problem hiding this comment.
I think this should also have CASCADE here
There was a problem hiding this comment.
This code path consumes DROP DATABASE which is parsed by the standard SQL parser. Is that wanted or should I remove this?
|
The remaining open question on this one is what to do with This has me wondering if we should use a different syntax to deal with connecting to and disconnecting from remote catalogs than |
| CREATE DATABASE cat; | ||
| ``` | ||
|
|
||
| ## CREATE EXTERNAL CATALOG |
There was a problem hiding this comment.
The documentation above uses CREATE DATABASE ... for internal catalogs.
Why the external ones use CATALOG instead of DATABASE ?
Maybe we should just mention that they are synonyms ?!
There was a problem hiding this comment.
I should have taken a closer look at the existing documentation. My bad.
The usage of this terminology is kind of inconsistent across the RDBMS ecosystem. The SQL spec talks about a hierarchy of catalogs, schemas, and schema objects (tables, views, etc.). What 'database' maps to seems to be implementation dependent. In Databricks and MySQL for instance, CREATE DATABASE is a synonym for CREATE SCHEMA.
The term catalog on the other hand maps quite nicely to Iceberg catalogs and many others (thank you for the list StarRocks). Looking at Iceberg, a database/schema maps to a namespace there.
But... DataFusion today treats CREATE DATABASE as a synonym for CREATE CATALOG. Not sure how best to proceed.
There was a problem hiding this comment.
A quick Claude generated survey that may or may not be useful:
Prompt
Are there any RDBMS implementations where CREATE DATABASE is a synonym for CREATE CATALOG rather than CREATE SCHEMA?
Answer
Not as a literal keyword synonym, as far as I know. I'm not aware of any mainstream RDBMS that accepts CREATE CATALOG as an alias for CREATE DATABASE. But the answer changes if you mean "a database that is semantically a catalog."
Where CREATE DATABASE creates a standard-SQL catalog (conceptually)
In the SQL standard's hierarchy (catalog → schema → object), several major systems make "database" the catalog level. None of them has a CREATE CATALOG statement.
PostgreSQL: a database holds schemas, and information_schema.tables.table_catalog reports the database name. CREATE DATABASE and CREATE SCHEMA are distinct.
SQL Server: same pattern. TABLE_CATALOG is the database, and objects are named database.schema.object.
Snowflake, Redshift, and similar warehouses: database.schema.table, with TABLE_CATALOG holding the database name.
MotherDuck/DuckDB: a MotherDuck CREATE DATABASE produces what DuckDB treats as a catalog (an attached database), and schemas live inside it.
Where CREATE DATABASE is the schema level
MySQL / MariaDB: CREATE SCHEMA is documented as a synonym for CREATE DATABASE.
Spark SQL / Databricks: CREATE DATABASE is an alias for CREATE SCHEMA. Unity Catalog adds a separate CREATE CATALOG above it.
Hive, Impala: DATABASE and SCHEMA are interchangeable.
Flink SQL, StarRocks, Doris: they have both CREATE CATALOG and CREATE DATABASE, with the database sitting inside the catalog (i.e., at the schema level).
A reverse oddity: Firebird/InterBase
Historically, CREATE SCHEMA was a synonym for CREATE DATABASE in these systems, and it created a whole database file. Firebird 6 introduces real schemas, which changes this.
So if you've seen something claim CREATE DATABASE ≡ CREATE CATALOG, it's most likely describing the semantic mapping in PostgreSQL/SQL Server-style systems rather than an actual syntactic alias. If you have a specific engine in mind, I can check its current docs, since some newer engines add syntax quickly.
I think we should just use CREATE CATALOG consistently in our docs / examples |
I agree -- this is fine. We can sort out the I think this pR is ready to go now -- and we can iterate on it in follow on PRs. Any other thoughts before we merge @pepijnve or @martin-g ? |
👍🏻
IMO it would be good to add a simple SLT to document/verify that they are synonyms: |
We can keep
I have the cascade part you requested ready locally, was just waiting on clarification regarding the direction. I'll finish that and add it to this PR.
I'm not inclined to add |
|
So, |
That's what 795f0de does indeed. I've biased everything towards One thing I'm not entirely sure about in that commit is the method name The alternative would be to make a distinction at registration time. In other words, we add an additional method |
|
FWIW, encountering things like this in the current code made the best choice seem obvious 
|
|
Test failure seems to be an unrelated minio problem |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I still have some questions but I think we have iterated on this enough in this PR and we can refine the design / implementation as a follow on PR
| /// to prevent a catalog from being dropped if it is not empty. | ||
| /// | ||
| /// By default returns a "Not Implemented" error | ||
| fn prepare_deregister_catalog(&self, _cascade: bool) -> Result<()> { |
There was a problem hiding this comment.
I don't fully understand why this can't be implemented in deregister_catalog (why couldn't a deregister_catalog call simply return an error if it wasn't empty 🤔 )
As you prefer. Happy to keep on tinkering on it. |
…talogs (apache#25112) ## Which issue does this PR close? - Closes apache#25111. ## Rationale for this change Adding a `CatalogProviderFactory` trait, similar to `TableProviderFactory`, makes it possible for extensions to register support for external catalogs that can then be used by consumers without requiring additional code. This is quite convenient when working with open table formats via catalogs. ## What changes are included in this PR? - Adds a `CatalogProviderFactory` trait similar to `TableProviderFactory` - Adds `CREATE EXTERNAL CATALOG` and `DROP CATALOG` support ## What is the testing strategy for this PR? - Covered by unit and integration tests ## Are there any user-facing changes? Yes, the supported DDL has been extended. Documentation has been updated to reflect this. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…talogs (apache#25112) ## Which issue does this PR close? - Closes apache#25111. ## Rationale for this change Adding a `CatalogProviderFactory` trait, similar to `TableProviderFactory`, makes it possible for extensions to register support for external catalogs that can then be used by consumers without requiring additional code. This is quite convenient when working with open table formats via catalogs. ## What changes are included in this PR? - Adds a `CatalogProviderFactory` trait similar to `TableProviderFactory` - Adds `CREATE EXTERNAL CATALOG` and `DROP CATALOG` support ## What is the testing strategy for this PR? - Covered by unit and integration tests ## Are there any user-facing changes? Yes, the supported DDL has been extended. Documentation has been updated to reflect this. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
6 participants
Which issue does this PR close?
Rationale for this change
Adding a
CatalogProviderFactorytrait, similar toTableProviderFactory, makes it possible for extensions to register support for external catalogs that can then be used by consumers without requiring additional code. This is quite convenient when working with open table formats via catalogs.What changes are included in this PR?
CatalogProviderFactorytrait similar toTableProviderFactoryCREATE EXTERNAL CATALOGandDROP CATALOGsupportWhat is the testing strategy for this PR?
Are there any user-facing changes?
Yes, the supported DDL has been extended. Documentation has been updated to reflect this.