You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "pepijnve (via GitHub)" <gi...@apache.org> on 2026/09/09 13:22:12 UTC
[PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
pepijnve opened a new pull request, #25112:
URL: https://github.com/apache/datafusion/pull/25112
## Which issue does this PR close?
- Closes #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071674001
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
@alamb wdyt, would it make sense to add a session level `SecretsStore` as an extra extension point and then wire SQL `create secret` and `drop secret` to those? I would deliberately leave this open ended and default to a simple in-memory hashmap. The main intended use is to provide a way to reuse keys for multiple connections and provide a way to inject them out-of-band.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4076148030
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
I realize your usecase may only be for read only access to a catalog in which case dropping it without cascade does make sense. But in general I think having the option for cascade would be good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5617882161
Moved back to draft; still some work to do based on the CI results.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071007062
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -89,3 +113,105 @@ async fn create_drop_table() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn create_external_catalog_with_factory() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071129764
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1043,6 +1063,49 @@ impl SessionContext {
}
}
+ async fn create_external_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<DataFrame> {
+ let exists = self.catalog(cmd.catalog_name.as_str()).is_some();
+
+ match (cmd.if_not_exists, cmd.or_replace, exists) {
+ (true, false, true) => self.return_empty_dataframe(),
+ (true, true, true) => {
+ exec_err!("'IF NOT EXISTS' cannot coexist with 'REPLACE'")
+ }
+ (false, false, true) => {
+ exec_err!("External catalog '{}' already exists", cmd.catalog_name)
+ }
+ (_, _, _) => {
+ let new_catalog = self.create_custom_catalog(cmd).await?;
+ self.state
+ .write()
+ .catalog_list()
+ .register_catalog(cmd.catalog_name.clone(), new_catalog);
+ self.return_empty_dataframe()
+ }
+ }
+ }
+
+ async fn create_custom_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<Arc<dyn CatalogProvider>> {
+ let state = self.state.read().clone();
+ let catalog_type = cmd.catalog_type.to_uppercase();
Review Comment:
Isn't it already uppercased by the parser ?
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
Are `or_replace` and `options` intentionally omitted here ?
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -494,6 +507,18 @@ impl SessionState {
&mut Arc::make_mut(&mut self.inner).table_factories
}
+ /// Get the catalog factories
+ pub fn catalog_factories(&self) -> &HashMap<String, Arc<dyn CatalogProviderFactory>> {
+ &self.inner.catalog_factories
+ }
+
+ /// Get the catalog factories
Review Comment:
```suggestion
/// Get the catalog factories mutably
```
To make the difference against `catalog_factories()` above
##########
datafusion/session/src/catalog.rs:
##########
@@ -233,6 +254,17 @@ impl dyn CatalogProviderList {
}
}
+/// A factory which creates [`CatalogProvider`]s at runtime given a URL.
Review Comment:
A `URL` ? It receives a Session and a CreateExternalCatalog command.
```suggestion
/// A factory which creates [`CatalogProvider`]s at runtime from a
/// [`CreateExternalCatalog`] command (`CREATE EXTERNAL CATALOG ... STORED AS <TYPE>`).
```
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -89,3 +113,104 @@ async fn create_drop_table() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn create_external_catalog_with_factory() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse' OPTIONS ('warehouse' 'cat')";
+ ctx.sql(sql).await?;
+
+ assert!(
+ ctx.catalog("cat").is_some(),
+ "Catalog should have been created!"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_unknown_factory() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse'";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(
+ err.to_string(),
+ "Unable to find catalog factory for TESTCATALOG"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_factory_error_not_registered() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x' OPTIONS ('fail' 'true')";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "catalog factory configured to fail");
+ assert!(
+ ctx.catalog("cat").is_none(),
+ "Catalog should not have been registered when the factory errors"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_if_not_exists() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ // creating it again without IF NOT EXISTS should fail
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "already exists");
+
+ // ... but should succeed with IF NOT EXISTS
+ let sql = "CREATE EXTERNAL CATALOG IF NOT EXISTS cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ Ok(())
+}
+
Review Comment:
Add a test for `OR REPLACE`:
```suggestion
#[tokio::test]
async fn create_external_catalog_or_replace() -> Result<()> {
let mut state = SessionStateBuilder::new().with_default_features().build();
state
.catalog_factories_mut()
.insert("TESTCATALOG".to_string(), Arc::new(TestCatalogFactory {}));
let ctx = SessionContext::new_with_state(state);
ctx.sql("CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'")
.await?;
let original = ctx.catalog("cat").unwrap();
ctx.sql("CREATE OR REPLACE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'")
.await?;
let replacement = ctx.catalog("cat").unwrap();
assert!(!Arc::ptr_eq(&original, &replacement));
Ok(())
}
```
##########
docs/source/user-guide/sql/ddl.md:
##########
@@ -35,6 +35,58 @@ CREATE DATABASE [ IF NOT EXISTS ] <i><b>catalog</i></b>
CREATE DATABASE cat;
```
+## CREATE EXTERNAL CATALOG
+
+`CREATE EXTERNAL CATALOG` registers a catalog built by a registered
+[`CatalogProviderFactory`], such as a catalog backed by a remote catalog
+service (for example, an Iceberg REST catalog), so that it can be queried
+alongside DataFusion's built-in catalogs. A `CatalogProviderFactory` must
+first be registered on the `SessionState` with a key matching the
+`STORED AS` clause below — see the [Catalog Provider Factories] section of
+the Library User Guide for how to implement and register one.
+
+The supported syntax is:
+
+```sql
+CREATE [OR REPLACE] EXTERNAL CATALOG
+[ IF NOT EXISTS ]
+<catalog_name>
+STORED AS <catalog_type>
+[ LOCATION <literal> ]
+[ OPTIONS (<key_value_list>) ]
+
+<key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+```
+
Review Comment:
```suggestion
`OR REPLACE` and `IF NOT EXISTS` cannot coexist.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4076140928
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
> Conversely cascade: false implicitly means restrict, but then I have no way to disconnect from the catalog unless it's empty.
I am not sure we need to implement any cascade semantics but I was triyng to design the API in such a way that we don't have to make a breaking change in the future
For example if someone wanted to create catalogs into which you could create tables they may want to have `CASCADE`
```sql
create catalog foo;
create schema foo.bar;
create table foo.bar.baz;
drop catalog foo; -- would fail because the catalog has a table
drop catalog foo cascade; // would recursively drop all schema / tables
```
So I was thinking if the API passed the flag through we could support this usecase
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070274878
##########
datafusion-examples/examples/sql_ops/custom_sql_parser.rs:
##########
@@ -16,11 +16,16 @@
// under the License.
//! This example demonstrates extending the DataFusion SQL parser to support
-//! custom DDL statements, specifically `CREATE EXTERNAL CATALOG`.
+//! custom DDL statements, specifically `CREATE FOREIGN 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.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070073638
##########
parquet-testing:
##########
Review Comment:
That was committed by accident. I'll be sure to revert that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4080319120
##########
docs/source/user-guide/sql/ddl.md:
##########
@@ -35,6 +35,58 @@ CREATE DATABASE [ IF NOT EXISTS ] <i><b>catalog</i></b>
CREATE DATABASE cat;
```
+## CREATE EXTERNAL CATALOG
Review 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 ?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5813377902
> > I think we should just use CREATE CATALOG consistently in our docs / examples
>
> 👍🏻 But `CREATE DATABASE ...` is already in the wild, so it has to be supported.
We can keep `CREATE DATABASE` support as is as an alias for `CREATE CATALOG`. And then use `CREATE CATALOG` everywhere in the documentation. The added benefit of using `catalog.schema.table` is that this maps nicely to the equivalent types in DataFusion.
> > 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 ?
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.
> IMO it would be good to add a simple SLT to document/verify that they are synonyms:
>
> ```
> statement ok
> CREATE EXTERNAL DATABASE abc; -- uses DATABASE
> ```
I'm not inclined to add `CREATE EXTERNAL DATABASE` support due to the inconsistent use of `DATABASE` across systems this is just going to be confusing. We can add a plain `CREATE DATABASE` test though to cover what you have in mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5814867009
> So, `CREATE DATABASE` will be supported as an alias/synonym to `CREATE CATALOG` but `CREATE EXTERNAL DATABASE` will not be supported ?
That's what https://github.com/apache/datafusion/pull/25112/commits/795f0de5bb57d511a2814b64cb7037f9f3b3dc17 does indeed. I've biased everything towards `CREATE CATALOG`, but retained `CREATE DATABASE` support and added the test you suggested.
One thing I'm not entirely sure about in that commit is the method name `CatalogProvider::prepare_deregister_catalog`. I wanted to be able to have a difference in behaviour when dropping an internal catalog vs dropping an external one. `DROP CATALOG <internal>` will error if the catalog is not empty. `DROP CATALOG <external>` should simply detach the catalog. Each implementation of `CatalogProvider` can choose what it does using this callback.
The alternative would be to make a distinction at registration time. In other words, we add an additional method `CatalogProviderList::register_external_catalog` so that `CatalogProviderList::deregister_catalog` implementations can decide what to do. I think this would still require a callback on `CatalogProvider` though since you want to be able to make the `DROP CATALOG ... CASCADE` operation an atomic operation which is going to be impossible to do externally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5814900470
FWIW, encountering things like this in the current code made the best choice seem obvious
<img width="288" height="85" alt="image" src="https://github.com/user-attachments/assets/33c1223f-e51f-488e-b194-b4a6b17c10eb" />
<img width="224" height="121" alt="image" src="https://github.com/user-attachments/assets/5c306830-8f79-40f3-bda6-61d3929c6df4" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25112:
URL: https://github.com/apache/datafusion/pull/25112
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5784485863
The remaining open question on this one is what to do with `drop catalog ... cascade`. `drop catalog ...` (which is equivalent to `drop catalog ... restrict`) would prevent non-empty external catalogs from being forgotten. `drop catalog ... cascade` would delete everything on your remote catalog.
This has me wondering if we should use a different syntax to deal with connecting to and disconnecting from remote catalogs than `create external catalog` and `drop catalog`. I went for that in this PR since it seemed the natural extrapolation of `create external table`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4082170169
##########
docs/source/user-guide/sql/ddl.md:
##########
@@ -35,6 +35,58 @@ CREATE DATABASE [ IF NOT EXISTS ] <i><b>catalog</i></b>
CREATE DATABASE cat;
```
+## CREATE EXTERNAL CATALOG
Review 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](https://docs.databricks.com/aws/en/sql/language-manual/sql-ref-syntax-ddl-create-database) and [MySQL](https://dev.mysql.com/doc/refman/8.4/en/create-database.html) for instance, `CREATE DATABASE` is a synonym for `CREATE SCHEMA`.
The term catalog on the other hand maps quite nicely to [Iceberg catalogs](https://iceberg.apache.org/terms/#catalog) and [many others](https://docs.starrocks.io/docs/data_source/catalog/) (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4082237936
##########
docs/source/user-guide/sql/ddl.md:
##########
@@ -35,6 +35,58 @@ CREATE DATABASE [ IF NOT EXISTS ] <i><b>catalog</i></b>
CREATE DATABASE cat;
```
+## CREATE EXTERNAL CATALOG
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5815731965
Test failure seems to be an unrelated minio problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5608837840
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v55.0.0 (current)
Built [ 63.410s] (current)
Parsing datafusion v55.0.0 (current)
Parsed [ 0.035s] (current)
Building datafusion v55.0.0 (baseline)
Built [ 63.050s] (baseline)
Parsing datafusion v55.0.0 (baseline)
Parsed [ 0.037s] (baseline)
Checking datafusion v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.610s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 128.910s] datafusion
Building datafusion-catalog v55.0.0 (current)
Built [ 43.155s] (current)
Parsing datafusion-catalog v55.0.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-catalog v55.0.0 (baseline)
Built [ 42.910s] (baseline)
Parsing datafusion-catalog v55.0.0 (baseline)
Parsed [ 0.025s] (baseline)
Checking datafusion-catalog v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.124s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 87.404s] datafusion-catalog
Building datafusion-expr v55.0.0 (current)
Built [ 31.440s] (current)
Parsing datafusion-expr v55.0.0 (current)
Parsed [ 0.082s] (current)
Building datafusion-expr v55.0.0 (baseline)
Built [ 32.053s] (baseline)
Parsing datafusion-expr v55.0.0 (baseline)
Parsed [ 0.082s] (baseline)
Checking datafusion-expr v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.228s] 223 checks: 221 pass, 1 fail, 1 warn, 31 skip
--- failure enum_variant_added: enum variant added on exhaustive enum ---
Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_variant_added.ron
Failed in:
variant DdlStatement:CreateExternalCatalog in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:54
variant DdlStatement:DropCatalog in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:64
variant DdlStatement:CreateExternalCatalog in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:54
variant DdlStatement:DropCatalog in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:64
--- warning partial_ord_enum_variants_reordered: enum variants reordered in #[derive(PartialOrd)] enum ---
Description:
A public enum that derives PartialOrd had its variants reordered. #[derive(PartialOrd)] uses the enum variant order to set the enum's ordering behavior, so this change may break downstream code that relies on the previous order.
ref: https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html#derivable
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/partial_ord_enum_variants_reordered.ron
Failed in:
DdlStatement::CreateIndex moved from position 6 to 7, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:56
DdlStatement::DropTable moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:58
DdlStatement::DropView moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:60
DdlStatement::DropCatalogSchema moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:62
DdlStatement::CreateFunction moved from position 10 to 12, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:67
DdlStatement::DropFunction moved from position 11 to 13, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:69
DdlStatement::CreateIndex moved from position 6 to 7, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:56
DdlStatement::DropTable moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:58
DdlStatement::DropView moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:60
DdlStatement::DropCatalogSchema moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:62
DdlStatement::CreateFunction moved from position 10 to 12, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:67
DdlStatement::DropFunction moved from position 11 to 13, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/ddl.rs:69
Summary semver requires new major version: 1 major and 0 minor checks failed
Warning produced 1 major and 0 minor level warnings
Finished [ 66.079s] datafusion-expr
Building datafusion-optimizer v55.0.0 (current)
Built [ 29.480s] (current)
Parsing datafusion-optimizer v55.0.0 (current)
Parsed [ 0.032s] (current)
Building datafusion-optimizer v55.0.0 (baseline)
Built [ 28.939s] (baseline)
Parsing datafusion-optimizer v55.0.0 (baseline)
Parsed [ 0.033s] (baseline)
Checking datafusion-optimizer v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.169s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 59.472s] datafusion-optimizer
Building datafusion-proto v55.0.0 (current)
Built [ 58.840s] (current)
Parsing datafusion-proto v55.0.0 (current)
Parsed [ 0.018s] (current)
Building datafusion-proto v55.0.0 (baseline)
Built [ 58.166s] (baseline)
Parsing datafusion-proto v55.0.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-proto v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.124s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 118.369s] datafusion-proto
Building datafusion-session v55.0.0 (current)
Built [ 41.476s] (current)
Parsing datafusion-session v55.0.0 (current)
Parsed [ 0.011s] (current)
Building datafusion-session v55.0.0 (baseline)
Built [ 41.495s] (baseline)
Parsing datafusion-session v55.0.0 (baseline)
Parsed [ 0.012s] (baseline)
Checking datafusion-session v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.183s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 84.182s] datafusion-session
Building datafusion-sql v55.0.0 (current)
Built [ 46.186s] (current)
Parsing datafusion-sql v55.0.0 (current)
Parsed [ 0.034s] (current)
Building datafusion-sql v55.0.0 (baseline)
Built [ 46.094s] (baseline)
Parsing datafusion-sql v55.0.0 (baseline)
Parsed [ 0.033s] (baseline)
Checking datafusion-sql v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.221s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure enum_variant_added: enum variant added on exhaustive enum ---
Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_variant_added.ron
Failed in:
variant Statement:CreateExternalCatalog in /home/runner/work/datafusion/datafusion/datafusion/sql/src/parser.rs:405
variant Statement:DropCatalog in /home/runner/work/datafusion/datafusion/datafusion/sql/src/parser.rs:407
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 94.119s] datafusion-sql
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5810913861
> I think we should just use CREATE CATALOG consistently in our docs / examples
👍🏻
But `CREATE DATABASE ...` is already in the wild, so it has to be supported.
> 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:
```
statement ok
CREATE EXTERNAL DATABASE abc; -- uses DATABASE
statement ok
DROP CATALOG abc; -- uses CATALOG
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070474674
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
While I was adding the `cascade` parameter I got stuck on describing its semantics. We're mainly trying to 'forget' a registered external connection to. What would the expected behaviour be if someone specifies `cascade: true` on an external iceberg catalog for instance? Conversely `cascade: false` implicitly means `restrict`, but then I have no way to disconnect from the catalog unless it's empty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071570370
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1043,6 +1063,49 @@ impl SessionContext {
}
}
+ async fn create_external_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<DataFrame> {
+ let exists = self.catalog(cmd.catalog_name.as_str()).is_some();
+
+ match (cmd.if_not_exists, cmd.or_replace, exists) {
+ (true, false, true) => self.return_empty_dataframe(),
+ (true, true, true) => {
+ exec_err!("'IF NOT EXISTS' cannot coexist with 'REPLACE'")
+ }
+ (false, false, true) => {
+ exec_err!("External catalog '{}' already exists", cmd.catalog_name)
+ }
+ (_, _, _) => {
+ let new_catalog = self.create_custom_catalog(cmd).await?;
+ self.state
+ .write()
+ .catalog_list()
+ .register_catalog(cmd.catalog_name.clone(), new_catalog);
+ self.return_empty_dataframe()
+ }
+ }
+ }
+
+ async fn create_custom_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<Arc<dyn CatalogProvider>> {
+ let state = self.state.read().clone();
+ let catalog_type = cmd.catalog_type.to_uppercase();
Review Comment:
This is mirroring what `create_custom_table` is doing. Fair question if this is wanted or not especially since the internal hash table can be acquired mutably so we can't control all inserts into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071590058
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -89,3 +113,104 @@ async fn create_drop_table() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn create_external_catalog_with_factory() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse' OPTIONS ('warehouse' 'cat')";
+ ctx.sql(sql).await?;
+
+ assert!(
+ ctx.catalog("cat").is_some(),
+ "Catalog should have been created!"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_unknown_factory() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse'";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(
+ err.to_string(),
+ "Unable to find catalog factory for TESTCATALOG"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_factory_error_not_registered() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x' OPTIONS ('fail' 'true')";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "catalog factory configured to fail");
+ assert!(
+ ctx.catalog("cat").is_none(),
+ "Catalog should not have been registered when the factory errors"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_if_not_exists() -> Result<()> {
+ let ctx: SessionContext = SessionStateBuilder::new()
+ .with_default_features()
+ .with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
+ .build()
+ .into();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ // creating it again without IF NOT EXISTS should fail
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "already exists");
+
+ // ... but should succeed with IF NOT EXISTS
+ let sql = "CREATE EXTERNAL CATALOG IF NOT EXISTS cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ Ok(())
+}
+
Review Comment:
Thanks for the suggestion. I've added this test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071608016
##########
datafusion/session/src/catalog.rs:
##########
@@ -233,6 +254,17 @@ impl dyn CatalogProviderList {
}
}
+/// A factory which creates [`CatalogProvider`]s at runtime given a URL.
Review Comment:
Another case of following the existing patterns:
```
/// A factory which creates [`TableProvider`]s at runtime given a URL.
```
Should I adapt both? I guess the intention was to refer to the location as the primary input rather than all the secondary bits that are in the command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5617936542
@geoffreyclaude @Jefffrey in #19383 an `create external catalog` example was added which is provided out of the box by this PR. I triggered an examples failure related to this. Any suggestions on how I should adapt the custom SQL parser example if that particular example is no longer actually custom?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5819907114
> 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
As you prefer. Happy to keep on tinkering on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4076152609
##########
datafusion/sql/src/statement.rs:
##########
@@ -814,6 +818,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
},
)))
}
+ ObjectType::Database => Ok(LogicalPlan::Ddl(
+ DdlStatement::DropCatalog(datafusion_expr::DropCatalog {
+ name: object_name_to_string(&name),
+ if_exists,
+ schema: DFSchemaRef::new(DFSchema::empty()),
Review Comment:
I think that is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5766503587
I plan to review this today
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071659276
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1043,6 +1063,49 @@ impl SessionContext {
}
}
+ async fn create_external_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<DataFrame> {
+ let exists = self.catalog(cmd.catalog_name.as_str()).is_some();
+
+ match (cmd.if_not_exists, cmd.or_replace, exists) {
+ (true, false, true) => self.return_empty_dataframe(),
+ (true, true, true) => {
+ exec_err!("'IF NOT EXISTS' cannot coexist with 'REPLACE'")
+ }
+ (false, false, true) => {
+ exec_err!("External catalog '{}' already exists", cmd.catalog_name)
+ }
+ (_, _, _) => {
+ let new_catalog = self.create_custom_catalog(cmd).await?;
+ self.state
+ .write()
+ .catalog_list()
+ .register_catalog(cmd.catalog_name.clone(), new_catalog);
+ self.return_empty_dataframe()
+ }
+ }
+ }
+
+ async fn create_custom_catalog(
+ &self,
+ cmd: &CreateExternalCatalog,
+ ) -> Result<Arc<dyn CatalogProvider>> {
+ let state = self.state.read().clone();
+ let catalog_type = cmd.catalog_type.to_uppercase();
Review Comment:
OK! Then let's keep it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "geoffreyclaude (via GitHub)" <gi...@apache.org>.
geoffreyclaude commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5678051454
> > @geoffreyclaude @Jefffrey in #19383 an `create external catalog` example was added which is provided out of the box by this PR. I triggered an examples failure related to this. Any suggestions on how I should adapt the custom SQL parser example if that particular example is no longer actually custom?
>
> For now I've adapted the example to use `CREATE FOREIGN CATALOG` as syntax, keeping everything else the same.
@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 `CREATE EXTERNAL CATALOG` support, and could probably be rewritten from scratch for some new SQL, but that's clearly out of scope of the PR.
I'd just suggest adding a short comment to the example explaining that `CREATE EXTERNAL CATALOG` is now natively supported, and this example uses `CREATE FOREIGN CATALOG` purely as demo, if that makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070954588
##########
datafusion/sql/src/parser.rs:
##########
@@ -1248,6 +1346,87 @@ impl<'a> DFParser<'a> {
Ok(Statement::CreateExternalTable(create))
}
+ /// Parses a `CREATE EXTERNAL CATALOG` statement, with `CREATE [OR
+ /// REPLACE] EXTERNAL CATALOG` already consumed.
+ fn parse_create_external_catalog(
+ &mut self,
+ or_replace: bool,
+ ) -> Result<Statement, DataFusionError> {
+ let if_not_exists =
+ self.parser
+ .parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
+
+ if if_not_exists && or_replace {
+ return parser_err!("'IF NOT EXISTS' cannot coexist with 'REPLACE'");
+ }
+
+ let catalog_name = self.parser.parse_object_name(true)?;
+
+ #[derive(Default)]
+ struct Builder {
Review Comment:
Nothing at all. AI generated artefact that I missed during review. I've removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070956672
##########
parquet-testing:
##########
Review Comment:
Reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071709920
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
> Agreed about the options! or_replace seems safe to be added.
I've added that for both catalog and table for now.
Revisiting this, the modelling (that already existed for external table) is kind of odd. We have two non-option bools which are actually mutually exclusive. This lets you represent 'if_not_exists: true, or_replace: true' which is a contradiction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4086109076
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
- filed https://github.com/apache/datafusion/issues/25662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5800367064
> But... DataFusion today treats CREATE DATABASE as a synonym for CREATE CATALOG. Not sure how best to proceed.
I think we should just use CREATE CATALOG consistently in our docs / examples
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4097087154
##########
datafusion/session/src/catalog.rs:
##########
@@ -171,6 +182,14 @@ pub trait CatalogProvider: Any + Debug + Sync + Send {
) -> Result<Option<Arc<dyn SchemaProvider>>> {
not_impl_err!("Deregistering new schemas is not supported")
}
+
+ /// Called when a catalog is being dropped. Implementations can use this hook
+ /// 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<()> {
Review 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 🤔 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5819860959
Thank you very much @pepijnve and @martin-g
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4066042034
##########
datafusion-examples/examples/sql_ops/custom_sql_parser.rs:
##########
@@ -296,28 +301,23 @@ impl<'a> CustomParser<'a> {
}
pub fn parse_statement(&mut self) -> Result<CustomStatement> {
- if self.is_create_external_catalog() {
- return self.parse_create_external_catalog();
+ if let Some(cfc) = self.parse_create_foreign_catalog()? {
+ return Ok(CustomStatement::CreateForeignCatalog(cfc));
}
+
Ok(CustomStatement::DFStatement(Box::new(
self.df_parser.parse_statement()?,
)))
}
- fn is_create_external_catalog(&self) -> bool {
- let t1 = &self.df_parser.parser.peek_nth_token(0).token;
- let t2 = &self.df_parser.parser.peek_nth_token(1).token;
- let t3 = &self.df_parser.parser.peek_nth_token(2).token;
-
- matches!(t1, Token::Word(w) if w.keyword == Keyword::CREATE)
- && matches!(t2, Token::Word(w) if w.keyword == Keyword::EXTERNAL)
- && matches!(t3, Token::Word(w) if w.value.to_uppercase() == "CATALOG")
- }
-
- fn parse_create_external_catalog(&mut self) -> Result<CustomStatement> {
- // Consume prefix tokens: CREATE EXTERNAL CATALOG
- for _ in 0..3 {
- self.df_parser.parser.next_token();
+ /// Parses a `CREATE FOREIGN CATALOG` statement.
+ fn parse_create_foreign_catalog(&mut self) -> Result<Option<CreateForeignCatalog>> {
+ if !self.df_parser.parser.parse_keywords(&[
Review Comment:
that is much nicer
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1556,6 +1584,27 @@ impl SessionStateBuilder {
self
}
+ /// Add a [`CatalogProviderFactory`] to the map of factories
+ pub fn with_catalog_factory(
+ mut self,
+ key: String,
+ catalog_factory: Arc<dyn CatalogProviderFactory>,
+ ) -> Self {
+ let mut catalog_factories = self.catalog_factories.unwrap_or_default();
Review Comment:
It would be nice to allow people to pass in `str` here rather than just String:
```suggestion
pub fn with_catalog_factory(
mut self,
key: impl Into<String>,
catalog_factory: Arc<dyn CatalogProviderFactory>,
) -> Self {
let key = key.into();
let mut catalog_factories = self.catalog_factories.unwrap_or_default();
```
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -89,3 +113,105 @@ async fn create_drop_table() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn create_external_catalog_with_factory() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
Review 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
```rust
let ctx: SessionContext = SessionStateBuilder::new()
.with_default_features()
.with_catalog_factory("TESTCATALOG", Arc::new(TestCatalogFactory {}))
.build()
.into();
```
##########
datafusion/sql/src/parser.rs:
##########
@@ -1248,6 +1346,87 @@ impl<'a> DFParser<'a> {
Ok(Statement::CreateExternalTable(create))
}
+ /// Parses a `CREATE EXTERNAL CATALOG` statement, with `CREATE [OR
+ /// REPLACE] EXTERNAL CATALOG` already consumed.
+ fn parse_create_external_catalog(
+ &mut self,
+ or_replace: bool,
+ ) -> Result<Statement, DataFusionError> {
+ let if_not_exists =
+ self.parser
+ .parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]);
+
+ if if_not_exists && or_replace {
+ return parser_err!("'IF NOT EXISTS' cannot coexist with 'REPLACE'");
+ }
+
+ let catalog_name = self.parser.parse_object_name(true)?;
+
+ #[derive(Default)]
+ struct Builder {
Review Comment:
What value does defining this struct add? It seems like all the fields are just referenced directly 🤔
##########
parquet-testing:
##########
Review Comment:
I don't think we should change the submodule pins in this PR
##########
datafusion-examples/examples/sql_ops/custom_sql_parser.rs:
##########
@@ -16,11 +16,16 @@
// under the License.
//! This example demonstrates extending the DataFusion SQL parser to support
-//! custom DDL statements, specifically `CREATE EXTERNAL CATALOG`.
+//! custom DDL statements, specifically `CREATE FOREIGN 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.
Review 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`)
```suggestion
//! 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.
```
##########
datafusion/core/tests/sql/create_drop.rs:
##########
@@ -89,3 +113,105 @@ async fn create_drop_table() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn create_external_catalog_with_factory() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
+ state
+ .catalog_factories_mut()
+ .insert("TESTCATALOG".to_string(), Arc::new(TestCatalogFactory {}));
+ let ctx = SessionContext::new_with_state(state);
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse' OPTIONS ('warehouse' 'cat')";
+ ctx.sql(sql).await?;
+
+ assert!(
+ ctx.catalog("cat").is_some(),
+ "Catalog should have been created!"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_unknown_factory() -> Result<()> {
+ let ctx = SessionContext::new();
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://bucket/warehouse'";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(
+ err.to_string(),
+ "Unable to find catalog factory for TESTCATALOG"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_factory_error_not_registered() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
+ state
+ .catalog_factories_mut()
+ .insert("TESTCATALOG".to_string(), Arc::new(TestCatalogFactory {}));
+ let ctx = SessionContext::new_with_state(state);
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x' OPTIONS ('fail' 'true')";
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "catalog factory configured to fail");
+ assert!(
+ ctx.catalog("cat").is_none(),
+ "Catalog should not have been registered when the factory errors"
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_external_catalog_if_not_exists() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
+ state
+ .catalog_factories_mut()
+ .insert("TESTCATALOG".to_string(), Arc::new(TestCatalogFactory {}));
+ let ctx = SessionContext::new_with_state(state);
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ // creating it again without IF NOT EXISTS should fail
+ let err = ctx.sql(sql).await.unwrap_err();
+ assert_contains!(err.to_string(), "already exists");
+
+ // ... but should succeed with IF NOT EXISTS
+ let sql = "CREATE EXTERNAL CATALOG IF NOT EXISTS cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn create_drop_catalog() -> Result<()> {
+ let mut state = SessionStateBuilder::new().with_default_features().build();
+ state
+ .catalog_factories_mut()
+ .insert("TESTCATALOG".to_string(), Arc::new(TestCatalogFactory {}));
+ let ctx = SessionContext::new_with_state(state);
+
+ let sql = "CREATE EXTERNAL CATALOG cat STORED AS TESTCATALOG LOCATION 's3://x'";
+ ctx.sql(sql).await?;
+ assert!(ctx.catalog("cat").is_some());
+
+ ctx.sql("DROP CATALOG cat").await?;
+ assert!(
+ ctx.catalog("cat").is_none(),
+ "Catalog should have been dropped!"
+ );
+
+ // dropping again should fail without IF EXISTS
Review Comment:
can you also please add a test for `DROP CATALOG cat CASCADE` ?
I would expect it to error with not supported / not implemented
##########
docs/source/library-user-guide/catalogs.md:
##########
@@ -305,6 +305,74 @@ impl CatalogProviderList for MemoryCatalogProviderList {
Like other traits, it also maintains the mapping of the Catalog's name to the CatalogProvider.
+## Catalog Provider Factories
+
+The catalogs above are all registered programmatically, ahead of time,
+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`
Review Comment:
👍
##########
datafusion/sql/src/parser.rs:
##########
@@ -1830,6 +2009,113 @@ mod tests {
Ok(())
}
+ fn make_create_external_catalog(catalog_type: &str) -> CreateExternalCatalog {
+ CreateExternalCatalog {
+ catalog_name: ObjectName::from(vec![Ident::from("c")]),
+ catalog_type: catalog_type.to_string(),
+ location: None,
+ if_not_exists: false,
+ or_replace: false,
+ options: vec![],
+ }
+ }
+
+ #[test]
+ fn create_external_catalog() -> Result<(), DataFusionError> {
+ // minimal: just STORED AS
+ let sql = "CREATE EXTERNAL CATALOG c STORED AS ICEBERG";
+ let expected = Statement::CreateExternalCatalog(CreateExternalCatalog {
+ catalog_type: "ICEBERG".to_string(),
+ ..make_create_external_catalog("ICEBERG")
+ });
+ expect_parse_ok(sql, expected)?;
+
+ // with LOCATION
+ let sql = "CREATE EXTERNAL CATALOG c STORED AS ICEBERG LOCATION 's3://bucket/warehouse'";
+ let expected = Statement::CreateExternalCatalog(CreateExternalCatalog {
+ location: Some("s3://bucket/warehouse".to_string()),
+ ..make_create_external_catalog("ICEBERG")
+ });
+ expect_parse_ok(sql, expected)?;
+
+ // with OPTIONS
+ let sql = "CREATE EXTERNAL CATALOG c STORED AS ICEBERG OPTIONS ('catalog.uri' 'http://rest:8181', 'warehouse' 'c')";
+ let expected = Statement::CreateExternalCatalog(CreateExternalCatalog {
+ options: vec![
+ (
+ "catalog.uri".into(),
+ Value::SingleQuotedString("http://rest:8181".into()),
+ ),
+ ("warehouse".into(), Value::SingleQuotedString("c".into())),
+ ],
+ ..make_create_external_catalog("ICEBERG")
+ });
+ expect_parse_ok(sql, expected)?;
+
+ // IF NOT EXISTS
+ let sql = "CREATE EXTERNAL CATALOG IF NOT EXISTS c STORED AS ICEBERG";
+ let expected = Statement::CreateExternalCatalog(CreateExternalCatalog {
+ if_not_exists: true,
+ ..make_create_external_catalog("ICEBERG")
+ });
+ expect_parse_ok(sql, expected)?;
+
+ // OR REPLACE
+ let sql = "CREATE OR REPLACE EXTERNAL CATALOG c STORED AS ICEBERG";
+ let expected = Statement::CreateExternalCatalog(CreateExternalCatalog {
+ or_replace: true,
+ ..make_create_external_catalog("ICEBERG")
+ });
+ expect_parse_ok(sql, expected)?;
+
+ // IF NOT EXISTS and OR REPLACE cannot coexist
+ expect_parse_error(
+ "CREATE OR REPLACE EXTERNAL CATALOG IF NOT EXISTS c STORED AS ICEBERG",
+ "'IF NOT EXISTS' cannot coexist with 'REPLACE'",
+ );
+
+ // missing STORED AS
+ expect_parse_error(
+ "CREATE EXTERNAL CATALOG c",
+ "Missing STORED AS clause in CREATE EXTERNAL CATALOG statement",
+ );
+
+ // UNBOUNDED is not applicable to catalogs
+ expect_parse_error(
+ "CREATE UNBOUNDED EXTERNAL CATALOG c STORED AS ICEBERG",
+ "UNBOUNDED is not supported for CREATE EXTERNAL CATALOG",
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn drop_catalog() -> Result<(), DataFusionError> {
+ let sql = "DROP CATALOG c";
Review Comment:
can you also please add a test for `DROP CATALOG c CASCADE` ?
##########
datafusion/sql/src/statement.rs:
##########
@@ -814,6 +818,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
},
)))
}
+ ObjectType::Database => Ok(LogicalPlan::Ddl(
+ DdlStatement::DropCatalog(datafusion_expr::DropCatalog {
+ name: object_name_to_string(&name),
+ if_exists,
+ schema: DFSchemaRef::new(DFSchema::empty()),
+ }),
+ )),
_ => not_impl_err!(
"Only `DROP TABLE/VIEW/SCHEMA ...` statement is supported currently"
Review Comment:
we should probably update this message to also mention catalog
##########
datafusion/sql/src/statement.rs:
##########
@@ -814,6 +818,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
},
)))
}
+ ObjectType::Database => Ok(LogicalPlan::Ddl(
+ DdlStatement::DropCatalog(datafusion_expr::DropCatalog {
+ name: object_name_to_string(&name),
+ if_exists,
+ schema: DFSchemaRef::new(DFSchema::empty()),
Review Comment:
I think this should also have `CASCADE` here
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
I think this should also take a `cascade` option for consistency with deregister_schema
https://github.com/apache/datafusion/blob/5efef8ee1337ff9449b062f7eaa7bea0322b3c5b/datafusion/catalog/src/memory/catalog.rs#L108-L129
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
+ Ok(())
+ }
+}
+
Review Comment:
As a follow on PR it might be nice to make a builder for `CreateExternalCatalog` and `DropCatalog`, similar to `CreateTable`
https://github.com/apache/datafusion/blob/5efef8ee1337ff9449b062f7eaa7bea0322b3c5b/datafusion/expr/src/logical_plan/ddl.rs#L283
We can file an issue as a follow on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5671273659
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25112?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `60.82474%` with `152 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.89%. Comparing base ([`6bbd3f4`](https://app.codecov.io/gh/apache/datafusion/commit/6bbd3f42c3e8a321b19f50253576e92e2765e5f4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`448ccab`](https://app.codecov.io/gh/apache/datafusion/commit/448ccab81a30344927f6953e6b97f84ce8fee834?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25112?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/sql/src/parser.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Fparser.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3BhcnNlci5ycw==) | 73.37% | [26 Missing and 19 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/expr/src/logical\_plan/ddl.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Flogical_plan%2Fddl.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vZGRsLnJz) | 9.30% | [39 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/core/src/execution/session\_state.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fexecution%2Fsession_state.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9leGVjdXRpb24vc2Vzc2lvbl9zdGF0ZS5ycw==) | 36.11% | [23 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/sql/src/statement.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fsql%2Fsrc%2Fstatement.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zcWwvc3JjL3N0YXRlbWVudC5ycw==) | 71.66% | [12 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/core/src/execution/context/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fexecution%2Fcontext%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9leGVjdXRpb24vY29udGV4dC9tb2QucnM=) | 79.31% | [11 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/session/src/catalog.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fsession%2Fsrc%2Fcatalog.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zZXNzaW9uL3NyYy9jYXRhbG9nLnJz) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/catalog/src/dynamic\_file/catalog.rs](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&filepath=datafusion%2Fcatalog%2Fsrc%2Fdynamic_file%2Fcatalog.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jYXRhbG9nL3NyYy9keW5hbWljX2ZpbGUvY2F0YWxvZy5ycw==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25112?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25112 +/- ##
==========================================
- Coverage 81.91% 81.89% -0.03%
==========================================
Files 1134 1134
Lines 425708 426079 +371
Branches 425708 426079 +371
==========================================
+ Hits 348726 348937 +211
- Misses 56305 56436 +131
- Partials 20677 20706 +29
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25112?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071668047
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
Agreed about the `options`!
`or_replace` seems safe to be added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071674001
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
@alamb wdyt, would it make sense to add a session level `SecretsStore` as an extra extension point and then wire SQL `create secret` and `drop secret` to those? I would deliberately leave this open ended and default to a simple in-memory hashmap. The main intended use is to provide a way to reuse keys for multiple connections and provide a way to inject them out-of-band.
See https://docs.snowflake.com/en/sql-reference/sql/create-secret, https://duckdb.org/docs/lts/configuration/secrets_manager, https://docs.risingwave.com/sql/commands/sql-create-secret, and https://learn.microsoft.com/en-us/sql/t-sql/statements/create-credential-transact-sql for some prior art.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4075358247
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
+ Ok(())
+ }
+}
+
Review Comment:
- Filed https://github.com/apache/datafusion/issues/25627
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4077121040
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
Would your proposal be for a `drop catalog external_catalog (restrict)` to just drop the connection? I was thinking 'well this should fail since the catalog is not empty', but I guess that's in line with `drop table external_table` simply forgetting the table and not actually deleting the backing file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5800386336
> This has me wondering if we should use a different syntax to deal with connecting to and disconnecting from remote catalogs than create external catalog and drop catalog. I went for that in this PR since it seemed the natural extrapolation of create external table.
I agree -- this is fine. We can sort out the `cascade` syntax / semantics as a follow on PR / work if the usecase arises.
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070474674
##########
datafusion/session/src/catalog.rs:
##########
@@ -206,6 +216,17 @@ pub trait CatalogProviderList: Any + Debug + Sync + Send {
catalog: Arc<dyn CatalogProvider>,
) -> Option<Arc<dyn CatalogProvider>>;
+ /// Removes a catalog from this list, returning it if it existed.
+ ///
+ /// Implementations of this method should return `Ok(None)` if no catalog
+ /// with `name` exists.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_catalog(&self, name: &str) -> Result<Option<Arc<dyn CatalogProvider>>> {
Review Comment:
While I was adding this I got stuck on describing the semantics of cascade. We're mainly trying to 'forget' a registered external connection to. What would the expected behaviour be if someone specifies `cascade: true` on an iceberg catalog for instance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071580003
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -494,6 +507,18 @@ impl SessionState {
&mut Arc::make_mut(&mut self.inner).table_factories
}
+ /// Get the catalog factories
+ pub fn catalog_factories(&self) -> &HashMap<String, Arc<dyn CatalogProviderFactory>> {
+ &self.inner.catalog_factories
+ }
+
+ /// Get the catalog factories
Review Comment:
Funny how well AI follows existing patterns. It clearly got this from `table_factories_mut`. If adjusted the doc for that function as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4075296586
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
> @alamb wdyt, would it make sense to add a session level SecretsStore as an extra extension point and then wire SQL create secret and drop secret to those? I would deliberately leave this open ended and default to a simple in-memory hashmap. The main intended use is to provide a way to reuse keys for multiple connections and provide a way to inject them out-of-band.
I think that would make a good follow on PR for sure. I will be happy to review it as I agree it is an important extension point as people work on integrating iceberg and other table providers
The trick would be to ensure that the secret store is sufficiently accessable to the various catalogs / providers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070919614
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1556,6 +1584,27 @@ impl SessionStateBuilder {
self
}
+ /// Add a [`CatalogProviderFactory`] to the map of factories
+ pub fn with_catalog_factory(
+ mut self,
+ key: String,
+ catalog_factory: Arc<dyn CatalogProviderFactory>,
+ ) -> Self {
+ let mut catalog_factories = self.catalog_factories.unwrap_or_default();
Review Comment:
Adjusted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4070323535
##########
datafusion/sql/src/statement.rs:
##########
@@ -814,6 +818,13 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
},
)))
}
+ ObjectType::Database => Ok(LogicalPlan::Ddl(
+ DdlStatement::DropCatalog(datafusion_expr::DropCatalog {
+ name: object_name_to_string(&name),
+ if_exists,
+ schema: DFSchemaRef::new(DFSchema::empty()),
Review Comment:
This code path consumes `DROP DATABASE` which is parsed by the standard SQL parser. Is that wanted or should I remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#discussion_r4071629685
##########
datafusion/sql/src/parser.rs:
##########
@@ -309,6 +309,70 @@ impl fmt::Display for CreateExternalTable {
}
}
+/// DataFusion extension `CREATE EXTERNAL CATALOG` statement.
+///
+/// ```sql
+/// CREATE [OR REPLACE] EXTERNAL CATALOG [IF NOT EXISTS] <catalog_name>
+/// STORED AS <catalog_type>
+/// [ LOCATION <literal> ]
+/// [ OPTIONS (<key_value_list>) ]
+///
+/// <key_value_list> := (<literal> <literal>, <literal> <literal>, ...)
+/// ```
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct CreateExternalCatalog {
+ /// Catalog name
+ pub catalog_name: ObjectName,
+ /// The key used to look up the registered `CatalogProviderFactory`
+ pub catalog_type: String,
+ /// The physical location of the catalog, if applicable
+ pub location: Option<String>,
+ /// Option to not error if catalog already exists
+ pub if_not_exists: bool,
+ /// Option to replace the catalog if it already exists
+ pub or_replace: bool,
+ /// Catalog(provider) specific options
+ pub options: Vec<(String, Value)>,
+}
+
+impl fmt::Display for CreateExternalCatalog {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ write!(f, "CREATE EXTERNAL CATALOG ")?;
+ if self.if_not_exists {
+ write!(f, "IF NOT EXISTS ")?;
+ }
+ write!(f, "{} ", self.catalog_name)?;
+ write!(f, "STORED AS {}", self.catalog_type)?;
+ if let Some(location) = &self.location {
+ write!(
+ f,
+ " LOCATION {}",
+ Value::SingleQuotedString(location.clone())
+ )?;
+ }
Review Comment:
This one was not 100% AI :) I did consider adding those, but then noticed that `create external table` omits them. In the usage of this functionality in my codebase I'm passing shared secrets via options for the time being. Might be a good idea to not print the options for now until there's a secrets manager available in DataFusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5814487979
So, `CREATE DATABASE` will be supported as an alias/synonym to `CREATE CATALOG` but `CREATE EXTERNAL DATABASE` will not be supported ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add an extension point to allow SQL based registration of external catalogs [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25112:
URL: https://github.com/apache/datafusion/pull/25112#issuecomment-5677985498
> @geoffreyclaude @Jefffrey in #19383 an `create external catalog` example was added which is provided out of the box by this PR. I triggered an examples failure related to this. Any suggestions on how I should adapt the custom SQL parser example if that particular example is no longer actually custom?
For now I've adapted the example to use `CREATE FOREIGN CATALOG` as syntax, keeping everything else the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org