You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "findepi (via GitHub)" <gi...@apache.org> on 2024/07/17 14:18:29 UTC
[PR] Extract catalog API to separate crate [datafusion]
findepi opened a new pull request, #11516:
URL: https://github.com/apache/datafusion/pull/11516
This moves `CatalogProvider`, `TableProvider`, `SchemaProvider` to a new `datafusion-catalog` crate. The circular dependency between core `SessionState` and implementations is broken up by introducing `CatalogSession` dyn trait. Implementations of `TableProvider` that reside under core current have access to `CatalogSession` by downcasting. This is supposed to be an intermediate step.
Fixes https://github.com/apache/datafusion/issues/10782
Relates to https://github.com/apache/datafusion/issues/11420
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248706460
( Squashed, rebased to resolve conflicts and force-pushed. No other changes. )
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686590748
##########
datafusion/core/src/lib.rs:
##########
@@ -535,6 +535,11 @@ pub use common::config;
// NB datafusion execution is re-exported in the `execution` module
+/// re-export of [`datafusion_catalog`] crate
+pub mod catalog_api {
Review Comment:
There already is `catalog` and still in use. it contains TableProvider implementations that come with core.
Do you mean the contents of proposed `catalog_api` should be imported into existing `catalog` module together with what core's module provides?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686729737
##########
datafusion/core/src/lib.rs:
##########
@@ -535,6 +535,11 @@ pub use common::config;
// NB datafusion execution is re-exported in the `execution` module
+/// re-export of [`datafusion_catalog`] crate
+pub mod catalog_api {
Review Comment:
pushed a rename (https://github.com/apache/datafusion/pull/11516#issuecomment-2243214905)
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686606121
##########
datafusion-cli/src/catalog.rs:
##########
@@ -237,7 +236,7 @@ fn substitute_tilde(cur: String) -> String {
mod tests {
use super::*;
- use datafusion::catalog::schema::SchemaProvider;
+ use datafusion::catalog_api::SchemaProvider;
Review Comment:
let's merge this thread with https://github.com/apache/datafusion/pull/11516#discussion_r1685354303
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248712084
> I see your PR plan to close #10782, isn't it the first step
you're right. unlinked.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248935316
Oh no, I broke the doc test -- sorry @findepi
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686740306
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -49,16 +49,18 @@ impl ListingTableFactory {
impl TableProviderFactory for ListingTableFactory {
async fn create(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
- let file_format = state
+ // TODO remove downcast_ref from here. Should file format factory be an extension to session state?
Review Comment:
also filed https://github.com/apache/datafusion/issues/11600 for this
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -736,13 +736,16 @@ impl TableProvider for ListingTable {
async fn scan(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
projection: Option<&Vec<usize>>,
filters: &[Expr],
limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
- let (mut partitioned_file_lists, statistics) =
- self.list_files_for_scan(state, filters, limit).await?;
+ // TODO remove downcast_ref from here?
Review Comment:
filed https://github.com/apache/datafusion/issues/11600 for downcasts
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685368085
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -736,13 +736,16 @@ impl TableProvider for ListingTable {
async fn scan(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
projection: Option<&Vec<usize>>,
filters: &[Expr],
limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
- let (mut partitioned_file_lists, statistics) =
- self.list_files_for_scan(state, filters, limit).await?;
+ // TODO remove downcast_ref from here?
Review Comment:
Yes, definitely we should remove the downcast (likely by adding some more functions to the `CatalogSession` trait) . But I also think we could do that as a follow on PR with minimal impact
Also this pattern shows that existing code could always use this pattern to downcast `&dy CatalogSession` to `&SessionState` which gives us an easy out for all migrations
--
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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2236188192
@jayzhan211 admittedly i made some choices about what to put into CatalogSession (or however we want to call this) and what to keep outside requiring a downcast to SessionState.
my line of thought was to let CatalogSession contain things that are separate modules and not optional dependencies -- so e.g. logical and physical expressions and plans were allowed base on that.
Of course, these choices remain pretty arbitrary. there may be things that CatalogSession should have but doesn't have yet, and also things that it got, but should be left out.
I will need your help, and others, to improve those decisions (provided this is the way to go at all).
--
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] Extract catalog API to separate crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2236357455
> let CatalogSession contain things that are separate modules and not optional dependencies
I think this is similar to minimum dependencies what I'm thinking. The only difference is that I propose `struct TableProviderContext`, but you have `trait CatalogSession`
--
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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2234372485
Eventually all green.
@jayzhan211 @alamb @comphead you may want to take a look
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1687040884
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
Review Comment:
performing the required remote catalog operations in parallel, and then plans the query
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2244060536
> the one about catalog names is applied, the one about separate traits is replied to (https://github.com/apache/datafusion/pull/11516#issuecomment-2243062441) and it looks we're good here.
I'm think of catalog-common in it's own crate, so others can only import the trait only for their own catalog provider instead of the whole builtin catalog 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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2238870621
> I think this is similar to minimum dependencies what I'm thinking. The only difference is that I propose `struct TableProviderContext`, but you have `trait CatalogSession`
>
> IMO struct is more sutiable for storing data, unlike trait that mainly used for sharing same behavior
A publicly constructible struct is indeed different than a trait. Not sure we want that though, as it would limit evolvability.
I thought about a trait, because of it resemblance to interfaces in other languages. To me seems suited for defining contracts -- and here we're defining a contract between core and the table providers.
I don't feel strongly though, even less so given my little experience
IMO a bigger question is what functionality does this new being provide.
If we go into composability, I would want to think about different table providers as add-ons or plugins.
Then, maybe FileFormatFactory could be moved our of core, as not inherently part of SQL execution?
This question would imply whether FileFormatFactory is exposed in TableProviderContext/CatalogSession.
cc @alamb it would be good to hear your opinion, especially that you created the https://github.com/apache/datafusion/issues/10782 issue, which this PR is in response to.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685421998
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
+/// using that snapshot.
+///
+/// # Example Catalog Implementations
+///
+/// Here are some examples of how to implement custom catalogs:
+///
+/// * [`datafusion-cli`]: [`DynamicFileCatalogProvider`] catalog provider
+/// that treats files and directories on a filesystem as tables.
+///
+/// * The [`catalog.rs`]: a simple directory based catalog.
+///
+/// * [delta-rs]: [`UnityCatalogProvider`] implementation that can
+/// read from Delta Lake tables
+///
+/// [`datafusion-cli`]: https://datafusion.apache.org/user-guide/cli/index.html
+/// [`DynamicFileCatalogProvider`]: https://github.com/apache/datafusion/blob/31b9b48b08592b7d293f46e75707aad7dadd7cbc/datafusion-cli/src/catalog.rs#L75
+/// [`catalog.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/catalog.rs
+/// [delta-rs]: https://github.com/delta-io/delta-rs
+/// [`UnityCatalogProvider`]: https://github.com/delta-io/delta-rs/blob/951436ecec476ce65b5ed3b58b50fb0846ca7b91/crates/deltalake-core/src/data_catalog/unity/datafusion.rs#L111-L123
+///
+/// [`TableProvider]: crate::datasource::TableProvider
+
+pub trait CatalogProvider: Sync + Send {
+ /// Returns the catalog provider as [`Any`]
+ /// so that it can be downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;
+
+ /// Retrieves a specific schema from the catalog by name, provided it exists.
+ fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>>;
+
+ /// Adds a new schema to this catalog.
+ ///
+ /// If a schema of the same name existed before, it is replaced in
+ /// the catalog and returned.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn register_schema(
+ &self,
+ name: &str,
Review Comment:
we usually add the underscore for unused variable`_name`.
--
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] Extract catalog API to separate crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2240801296
I plan to check this PR out carefully tomorrow
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2265355871
Are we tracking this TODO?
https://github.com/apache/datafusion/blob/0332eb569a5428ac385fe892ce7b5fb40d52c8c0/datafusion/core/src/datasource/listing_table_factory.rs#L55
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248817038
> Looking at this with fresh eyes. Is the CatalogSession name the best name we can have?
I thought about this a bit. How about `SessionStateProvider`?
That follows convention of other `Provider` traits in DataFusion, eg https://docs.rs/datafusion/latest/datafusion/index.html?search=provider, and is makes it pretty clear that this is connected to `SessionState`
Here are some other options:
* `PlanningState` or `SessionPlanningState` - the idea being this structure contains the information needed to plan the query
* `SessionPlanningState`
--
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] Extract catalog API to separate crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2239052594
I think there is clear dependency something close to
`Catalog` -> `Schema` -> `Table` -> `FileFormat` -> `QueryPlanner`.
They all have trait and trait implementation and we can easily downcast the trait to actual struct in the lower crate.
```rust
// queryplanner crate
trait QueryPlanner {}
impl QueryPlanner for DefaultPlanner {}
// fileformat crate
trait FileFormat {}
impl FileFormat for DefaultFile {
fn get_planner(&self, plan) -> Arc<QueryPlanner> { // able to downcast to planner }
}
// Table crate
trait TableProvider {}
impl TableProvider for DefaultTable {
// arguments are all higher level trait without circular dependency
fn get_file(&self, plan) -> Arc<QueryPlanner> { // able to downcast to actual planner }
fn get_planner(&self, fileformat) -> Arc<FileFormat> { // able to downcast to actual format}
}
```
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2241077155
BTW I agree with most of @jayzhan211 's ideas above, but in my mind they are follow ons / orthogonal to moving the catalog traits out of the core (as in we can do both!)
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2243060801
Squashed current code and rebased to resolve conflicts with `main`.
No other changes yet.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686735612
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
+ /// Return the session ID
+ fn session_id(&self) -> &str;
+
+ /// Return the [`SessionConfig`]
+ fn config(&self) -> &SessionConfig;
+
+ /// return the configuration options
+ fn config_options(&self) -> &ConfigOptions {
+ self.config().options()
+ }
+
+ /// Creates a physical [`ExecutionPlan`] plan from a [`LogicalPlan`].
+ ///
+ /// Note: this will optimize the provided plan first.
+ ///
+ /// This function will error for [`LogicalPlan`]s such as catalog DDL like
+ /// `CREATE TABLE`, which do not have corresponding physical plans and must
+ /// be handled by another layer, typically the `SessionContext`.
+ async fn create_physical_plan(
+ &self,
+ logical_plan: &LogicalPlan,
+ ) -> Result<Arc<dyn ExecutionPlan>>;
+
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not simplified or otherwise optimized: `a = 1
+ /// + 2` will not be simplified to `a = 3` as this is a more involved process.
+ /// See the [expr_api] example for how to simplify expressions.
+ ///
+ /// [expr_api]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/expr_api.rs
+ fn create_physical_expr(
+ &self,
+ expr: Expr,
+ df_schema: &DFSchema,
+ ) -> Result<Arc<dyn PhysicalExpr>>;
+
+ /// Return reference to scalar_functions
Review Comment:
https://github.com/apache/datafusion/issues/11599
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686603262
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -736,13 +736,16 @@ impl TableProvider for ListingTable {
async fn scan(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
projection: Option<&Vec<usize>>,
filters: &[Expr],
limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
- let (mut partitioned_file_lists, statistics) =
- self.list_files_for_scan(state, filters, limit).await?;
+ // TODO remove downcast_ref from here?
Review Comment:
this is a good point about the migrations
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2289862924
Sorry -- thanks for the heads up @phillipleblanc
--
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] Extract catalog API to separate crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2234919555
I think `CatalogSession` is not the minimum dependency, what is the reason to replace all the SessionState with CatalogSession
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2251433906
( squashed and rebased, no other changes )
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1690295912
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
Review Comment:
Good catch! This is verbatim moved from other place.
Unless the PRs are merged using rebase (keeping commit history), let me fix this in a follow-up.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1690406838
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
Here is a proposed PR to this branch to improve the comments: https://github.com/findepi/datafusion/pull/4 with a doc example
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2243214905
Pushed a commit renaming catalog_api to catalog
and pre-existing catalog to catalog_common.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248714765
Looking at this with fresh eyes. Is the `CatalogSession` name the best name we can have?
for example, could just `Session` be a better name for this new trait?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248780826
I am taking another pass at this one
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2243062441
> we might need more granular trait than single CatalogSession for each trait
I agree with you @jayzhan211 that having separate traits for TableProvider, CatalogProvider, SchemaProvider is the most flexible approach. Whether additional flexibility is worth additional complexity cost should depend on whether the additional flexibility provides actual value. For example, if all these traits are always implemented in core and always based on `SessionState`, then more flexibility didn't buy us much. However, if one could use TableProvider without CatalogProvider (or vice versa), or could use this components without core (and thus without `SessionState`), then additional flexibility would mean smaller interface surface to implement. Does this make 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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1687354649
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -49,16 +49,18 @@ impl ListingTableFactory {
impl TableProviderFactory for ListingTableFactory {
async fn create(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
- let file_format = state
+ // TODO remove downcast_ref from here. Should file format factory be an extension to session state?
Review Comment:
Moving datasource in it's own crate is our plan.
hive could be one of it too
- datafusion-datasource-parquet
- datafusion-datasource-csv
- datafusion-datasource-json
- datafusion-datasource-hive
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2265372798
@berkaysynnada yes, https://github.com/apache/datafusion/issues/11600
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685421598
##########
Cargo.toml:
##########
@@ -20,6 +20,7 @@ exclude = ["datafusion-cli", "dev/depcheck"]
members = [
"datafusion/common",
"datafusion/common-runtime",
+ "datafusion/catalog",
Review Comment:
nit: should we rename it to catalog-common, so we can reserve `catalog` for the actual catalog implementation
--
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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1681386777
##########
datafusion/catalog/src/table.rs:
##########
@@ -0,0 +1,293 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+use crate::session::CatalogSession;
+use arrow_schema::SchemaRef;
+use async_trait::async_trait;
+use datafusion_common::Result;
+use datafusion_common::{not_impl_err, Constraints, Statistics};
+use datafusion_expr::{
+ CreateExternalTable, Expr, LogicalPlan, TableProviderFilterPushDown, TableType,
+};
+use datafusion_physical_plan::ExecutionPlan;
+
+/// Source table
+#[async_trait]
+pub trait TableProvider: Sync + Send {
+ /// Returns the table provider as [`Any`](std::any::Any) so that it can be
+ /// downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Get a reference to the schema for this table
+ fn schema(&self) -> SchemaRef;
+
+ /// Get a reference to the constraints of the table.
+ /// Returns:
+ /// - `None` for tables that do not support constraints.
+ /// - `Some(&Constraints)` for tables supporting constraints.
+ /// Therefore, a `Some(&Constraints::empty())` return value indicates that
+ /// this table supports constraints, but there are no constraints.
+ fn constraints(&self) -> Option<&Constraints> {
+ None
+ }
+
+ /// Get the type of this table for metadata/catalog purposes.
+ fn table_type(&self) -> TableType;
+
+ /// Get the create statement used to create this table, if available.
+ fn get_table_definition(&self) -> Option<&str> {
+ None
+ }
+
+ /// Get the [`LogicalPlan`] of this table, if available
+ fn get_logical_plan(&self) -> Option<&LogicalPlan> {
+ None
+ }
+
+ /// Get the default value for a column, if available.
+ fn get_column_default(&self, _column: &str) -> Option<&Expr> {
+ None
+ }
+
+ /// Create an [`ExecutionPlan`] for scanning the table with optionally
+ /// specified `projection`, `filter` and `limit`, described below.
+ ///
+ /// The `ExecutionPlan` is responsible scanning the datasource's
+ /// partitions in a streaming, parallelized fashion.
+ ///
+ /// # Projection
+ ///
+ /// If specified, only a subset of columns should be returned, in the order
+ /// specified. The projection is a set of indexes of the fields in
+ /// [`Self::schema`].
+ ///
+ /// DataFusion provides the projection to scan only the columns actually
+ /// used in the query to improve performance, an optimization called
+ /// "Projection Pushdown". Some datasources, such as Parquet, can use this
+ /// information to go significantly faster when only a subset of columns is
+ /// required.
+ ///
+ /// # Filters
+ ///
+ /// A list of boolean filter [`Expr`]s to evaluate *during* the scan, in the
+ /// manner specified by [`Self::supports_filters_pushdown`]. Only rows for
+ /// which *all* of the `Expr`s evaluate to `true` must be returned (aka the
+ /// expressions are `AND`ed together).
+ ///
+ /// To enable filter pushdown you must override
+ /// [`Self::supports_filters_pushdown`] as the default implementation does
+ /// not and `filters` will be empty.
+ ///
+ /// DataFusion pushes filtering into the scans whenever possible
+ /// ("Filter Pushdown"), and depending on the format and the
+ /// implementation of the format, evaluating the predicate during the scan
+ /// can increase performance significantly.
+ ///
+ /// ## Note: Some columns may appear *only* in Filters
+ ///
+ /// In certain cases, a query may only use a certain column in a Filter that
+ /// has been completely pushed down to the scan. In this case, the
+ /// projection will not contain all the columns found in the filter
+ /// expressions.
+ ///
+ /// For example, given the query `SELECT t.a FROM t WHERE t.b > 5`,
+ ///
+ /// ```text
+ /// ┌────────────────────┐
+ /// │ Projection(t.a) │
+ /// └────────────────────┘
+ /// ▲
+ /// │
+ /// │
+ /// ┌────────────────────┐ Filter ┌────────────────────┐ Projection ┌────────────────────┐
+ /// │ Filter(t.b > 5) │────Pushdown──▶ │ Projection(t.a) │ ───Pushdown───▶ │ Projection(t.a) │
+ /// └────────────────────┘ └────────────────────┘ └────────────────────┘
+ /// ▲ ▲ ▲
+ /// │ │ │
+ /// │ │ ┌────────────────────┐
+ /// ┌────────────────────┐ ┌────────────────────┐ │ Scan │
+ /// │ Scan │ │ Scan │ │ filter=(t.b > 5) │
+ /// └────────────────────┘ │ filter=(t.b > 5) │ │ projection=(t.a) │
+ /// └────────────────────┘ └────────────────────┘
+ ///
+ /// Initial Plan If `TableProviderFilterPushDown` Projection pushdown notes that
+ /// returns true, filter pushdown the scan only needs t.a
+ /// pushes the filter into the scan
+ /// BUT internally evaluating the
+ /// predicate still requires t.b
+ /// ```
+ ///
+ /// # Limit
+ ///
+ /// If `limit` is specified, must only produce *at least* this many rows,
+ /// (though it may return more). Like Projection Pushdown and Filter
+ /// Pushdown, DataFusion pushes `LIMIT`s as far down in the plan as
+ /// possible, called "Limit Pushdown" as some sources can use this
+ /// information to improve their performance. Note that if there are any
+ /// Inexact filters pushed down, the LIMIT cannot be pushed down. This is
+ /// because inexact filters do not guarantee that every filtered row is
+ /// removed, so applying the limit could lead to too few rows being available
+ /// to return as a final result.
+ async fn scan(
+ &self,
+ state: &dyn CatalogSession,
+ projection: Option<&Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
+ ) -> Result<Arc<dyn ExecutionPlan>>;
+
+ /// Specify if DataFusion should provide filter expressions to the
+ /// TableProvider to apply *during* the scan.
+ ///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
+ /// # Parameters and Return Value
+ ///
+ /// The return `Vec` must have one element for each element of the `filters`
+ /// argument. The value of each element indicates if the TableProvider can
+ /// apply the corresponding filter during the scan. The position in the return
+ /// value corresponds to the expression in the `filters` parameter.
+ ///
+ /// If the length of the resulting `Vec` does not match the `filters` input
+ /// an error will be thrown.
+ ///
+ /// Each element in the resulting `Vec` is one of the following:
+ /// * [`Exact`] or [`Inexact`]: The TableProvider can apply the filter
+ /// during scan
+ /// * [`Unsupported`]: The TableProvider cannot apply the filter during scan
+ ///
+ /// By default, this function returns [`Unsupported`] for all filters,
+ /// meaning no filters will be provided to [`Self::scan`].
+ ///
+ /// [`Unsupported`]: TableProviderFilterPushDown::Unsupported
+ /// [`Exact`]: TableProviderFilterPushDown::Exact
+ /// [`Inexact`]: TableProviderFilterPushDown::Inexact
+ /// # Example
+ ///
+ /// ```rust-TODO-fix-this-example
Review Comment:
This needs to be addressed.
--
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] Extract catalog API to separate crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2239003792
I doubt that `CatalogSession` solves all the dependencies issue. `CatalogSession` is one higher level trait out of core. Others implementations are still leave inside core. If we want to move TableProvider implementation out of core into `datasource`, since we still need to downcast `CatalogSession` to `SessionState`, thus `SessionState` should be in the same as `datasource`, the same issue applied to other actual implementation like Catalog. We can actually bring `TableProvider`+`CatalogProvider`+`SessionState`+Others related things out of core into it's own crate without `CatalogSession`, but the dependencies between `TableProvider`, `CatalogProvider` and `SessionState` are what left to be decoupled. And, I don't think `CatalogSession` is enough 🤔 Did I miss something?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685352155
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
+ /// Return the session ID
+ fn session_id(&self) -> &str;
+
+ /// Return the [`SessionConfig`]
+ fn config(&self) -> &SessionConfig;
+
+ /// return the configuration options
+ fn config_options(&self) -> &ConfigOptions {
+ self.config().options()
+ }
+
+ /// Creates a physical [`ExecutionPlan`] plan from a [`LogicalPlan`].
+ ///
+ /// Note: this will optimize the provided plan first.
+ ///
+ /// This function will error for [`LogicalPlan`]s such as catalog DDL like
+ /// `CREATE TABLE`, which do not have corresponding physical plans and must
+ /// be handled by another layer, typically the `SessionContext`.
+ async fn create_physical_plan(
+ &self,
+ logical_plan: &LogicalPlan,
+ ) -> Result<Arc<dyn ExecutionPlan>>;
+
+ /// Create a [`PhysicalExpr`] from an [`Expr`] after applying type
+ /// coercion, and function rewrites.
+ ///
+ /// Note: The expression is not simplified or otherwise optimized: `a = 1
+ /// + 2` will not be simplified to `a = 3` as this is a more involved process.
+ /// See the [expr_api] example for how to simplify expressions.
+ ///
+ /// [expr_api]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/expr_api.rs
+ fn create_physical_expr(
+ &self,
+ expr: Expr,
+ df_schema: &DFSchema,
+ ) -> Result<Arc<dyn PhysicalExpr>>;
+
+ /// Return reference to scalar_functions
Review Comment:
As a follow on PR we may want to consider having `SessionCatalog` also implement `FunctionRegistry` rather than providing access to the hash maps directly
https://docs.rs/datafusion/latest/datafusion/execution/trait.FunctionRegistry.html
So that would mean
```rust
pub trait CatalogSession: FunctionRegistry + Send + Sync {
...
}
```
And removing these functions
However, for this PR I think keeping the API as close as possible to SessionState would be best
##########
datafusion/core/src/lib.rs:
##########
@@ -535,6 +535,11 @@ pub use common::config;
// NB datafusion execution is re-exported in the `execution` module
+/// re-export of [`datafusion_catalog`] crate
+pub mod catalog_api {
Review Comment:
perhaps this should be `catalog` rather than `catalog_api`
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
This is actually quite clever -- I didn't realize that the API of `SessionState` used by the TableProvider was so small.
##########
datafusion/catalog/src/table.rs:
##########
@@ -0,0 +1,292 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+use crate::session::CatalogSession;
+use arrow_schema::SchemaRef;
+use async_trait::async_trait;
+use datafusion_common::Result;
+use datafusion_common::{not_impl_err, Constraints, Statistics};
+use datafusion_expr::{
+ CreateExternalTable, Expr, LogicalPlan, TableProviderFilterPushDown, TableType,
+};
+use datafusion_physical_plan::ExecutionPlan;
+
+/// Source table
+#[async_trait]
+pub trait TableProvider: Sync + Send {
+ /// Returns the table provider as [`Any`](std::any::Any) so that it can be
+ /// downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Get a reference to the schema for this table
+ fn schema(&self) -> SchemaRef;
+
+ /// Get a reference to the constraints of the table.
+ /// Returns:
+ /// - `None` for tables that do not support constraints.
+ /// - `Some(&Constraints)` for tables supporting constraints.
+ /// Therefore, a `Some(&Constraints::empty())` return value indicates that
+ /// this table supports constraints, but there are no constraints.
+ fn constraints(&self) -> Option<&Constraints> {
+ None
+ }
+
+ /// Get the type of this table for metadata/catalog purposes.
+ fn table_type(&self) -> TableType;
+
+ /// Get the create statement used to create this table, if available.
+ fn get_table_definition(&self) -> Option<&str> {
+ None
+ }
+
+ /// Get the [`LogicalPlan`] of this table, if available
+ fn get_logical_plan(&self) -> Option<&LogicalPlan> {
+ None
+ }
+
+ /// Get the default value for a column, if available.
+ fn get_column_default(&self, _column: &str) -> Option<&Expr> {
+ None
+ }
+
+ /// Create an [`ExecutionPlan`] for scanning the table with optionally
+ /// specified `projection`, `filter` and `limit`, described below.
+ ///
+ /// The `ExecutionPlan` is responsible scanning the datasource's
+ /// partitions in a streaming, parallelized fashion.
+ ///
+ /// # Projection
+ ///
+ /// If specified, only a subset of columns should be returned, in the order
+ /// specified. The projection is a set of indexes of the fields in
+ /// [`Self::schema`].
+ ///
+ /// DataFusion provides the projection to scan only the columns actually
+ /// used in the query to improve performance, an optimization called
+ /// "Projection Pushdown". Some datasources, such as Parquet, can use this
+ /// information to go significantly faster when only a subset of columns is
+ /// required.
+ ///
+ /// # Filters
+ ///
+ /// A list of boolean filter [`Expr`]s to evaluate *during* the scan, in the
+ /// manner specified by [`Self::supports_filters_pushdown`]. Only rows for
+ /// which *all* of the `Expr`s evaluate to `true` must be returned (aka the
+ /// expressions are `AND`ed together).
+ ///
+ /// To enable filter pushdown you must override
+ /// [`Self::supports_filters_pushdown`] as the default implementation does
+ /// not and `filters` will be empty.
+ ///
+ /// DataFusion pushes filtering into the scans whenever possible
+ /// ("Filter Pushdown"), and depending on the format and the
+ /// implementation of the format, evaluating the predicate during the scan
+ /// can increase performance significantly.
+ ///
+ /// ## Note: Some columns may appear *only* in Filters
+ ///
+ /// In certain cases, a query may only use a certain column in a Filter that
+ /// has been completely pushed down to the scan. In this case, the
+ /// projection will not contain all the columns found in the filter
+ /// expressions.
+ ///
+ /// For example, given the query `SELECT t.a FROM t WHERE t.b > 5`,
+ ///
+ /// ```text
+ /// ┌────────────────────┐
+ /// │ Projection(t.a) │
+ /// └────────────────────┘
+ /// ▲
+ /// │
+ /// │
+ /// ┌────────────────────┐ Filter ┌────────────────────┐ Projection ┌────────────────────┐
+ /// │ Filter(t.b > 5) │────Pushdown──▶ │ Projection(t.a) │ ───Pushdown───▶ │ Projection(t.a) │
+ /// └────────────────────┘ └────────────────────┘ └────────────────────┘
+ /// ▲ ▲ ▲
+ /// │ │ │
+ /// │ │ ┌────────────────────┐
+ /// ┌────────────────────┐ ┌────────────────────┐ │ Scan │
+ /// │ Scan │ │ Scan │ │ filter=(t.b > 5) │
+ /// └────────────────────┘ │ filter=(t.b > 5) │ │ projection=(t.a) │
+ /// └────────────────────┘ └────────────────────┘
+ ///
+ /// Initial Plan If `TableProviderFilterPushDown` Projection pushdown notes that
+ /// returns true, filter pushdown the scan only needs t.a
+ /// pushes the filter into the scan
+ /// BUT internally evaluating the
+ /// predicate still requires t.b
+ /// ```
+ ///
+ /// # Limit
+ ///
+ /// If `limit` is specified, must only produce *at least* this many rows,
+ /// (though it may return more). Like Projection Pushdown and Filter
+ /// Pushdown, DataFusion pushes `LIMIT`s as far down in the plan as
+ /// possible, called "Limit Pushdown" as some sources can use this
+ /// information to improve their performance. Note that if there are any
+ /// Inexact filters pushed down, the LIMIT cannot be pushed down. This is
+ /// because inexact filters do not guarantee that every filtered row is
+ /// removed, so applying the limit could lead to too few rows being available
+ /// to return as a final result.
+ async fn scan(
+ &self,
+ state: &dyn CatalogSession,
Review Comment:
This one line I think is the biggest potential change in this PR as it will require changing all existing TableProviders which is one of the oldest and most widely used APIs in DataFusion
If we are ok with this change, I think the rest of this PR is wonderful
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1688021260
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
> I think prior to merging this PR we should document this trait a bit
💯 Given it will be a breaking change for many people - would be great to have some good docs on it.
##########
datafusion/catalog/src/table.rs:
##########
@@ -0,0 +1,292 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+use crate::session::CatalogSession;
+use arrow_schema::SchemaRef;
+use async_trait::async_trait;
+use datafusion_common::Result;
+use datafusion_common::{not_impl_err, Constraints, Statistics};
+use datafusion_expr::{
+ CreateExternalTable, Expr, LogicalPlan, TableProviderFilterPushDown, TableType,
+};
+use datafusion_physical_plan::ExecutionPlan;
+
+/// Source table
+#[async_trait]
+pub trait TableProvider: Sync + Send {
+ /// Returns the table provider as [`Any`](std::any::Any) so that it can be
+ /// downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Get a reference to the schema for this table
+ fn schema(&self) -> SchemaRef;
+
+ /// Get a reference to the constraints of the table.
+ /// Returns:
+ /// - `None` for tables that do not support constraints.
+ /// - `Some(&Constraints)` for tables supporting constraints.
+ /// Therefore, a `Some(&Constraints::empty())` return value indicates that
+ /// this table supports constraints, but there are no constraints.
+ fn constraints(&self) -> Option<&Constraints> {
+ None
+ }
+
+ /// Get the type of this table for metadata/catalog purposes.
+ fn table_type(&self) -> TableType;
+
+ /// Get the create statement used to create this table, if available.
+ fn get_table_definition(&self) -> Option<&str> {
+ None
+ }
+
+ /// Get the [`LogicalPlan`] of this table, if available
+ fn get_logical_plan(&self) -> Option<&LogicalPlan> {
+ None
+ }
+
+ /// Get the default value for a column, if available.
+ fn get_column_default(&self, _column: &str) -> Option<&Expr> {
+ None
+ }
+
+ /// Create an [`ExecutionPlan`] for scanning the table with optionally
+ /// specified `projection`, `filter` and `limit`, described below.
+ ///
+ /// The `ExecutionPlan` is responsible scanning the datasource's
+ /// partitions in a streaming, parallelized fashion.
+ ///
+ /// # Projection
+ ///
+ /// If specified, only a subset of columns should be returned, in the order
+ /// specified. The projection is a set of indexes of the fields in
+ /// [`Self::schema`].
+ ///
+ /// DataFusion provides the projection to scan only the columns actually
+ /// used in the query to improve performance, an optimization called
+ /// "Projection Pushdown". Some datasources, such as Parquet, can use this
+ /// information to go significantly faster when only a subset of columns is
+ /// required.
+ ///
+ /// # Filters
+ ///
+ /// A list of boolean filter [`Expr`]s to evaluate *during* the scan, in the
+ /// manner specified by [`Self::supports_filters_pushdown`]. Only rows for
+ /// which *all* of the `Expr`s evaluate to `true` must be returned (aka the
+ /// expressions are `AND`ed together).
+ ///
+ /// To enable filter pushdown you must override
+ /// [`Self::supports_filters_pushdown`] as the default implementation does
+ /// not and `filters` will be empty.
+ ///
+ /// DataFusion pushes filtering into the scans whenever possible
+ /// ("Filter Pushdown"), and depending on the format and the
+ /// implementation of the format, evaluating the predicate during the scan
+ /// can increase performance significantly.
+ ///
+ /// ## Note: Some columns may appear *only* in Filters
+ ///
+ /// In certain cases, a query may only use a certain column in a Filter that
+ /// has been completely pushed down to the scan. In this case, the
+ /// projection will not contain all the columns found in the filter
+ /// expressions.
+ ///
+ /// For example, given the query `SELECT t.a FROM t WHERE t.b > 5`,
+ ///
+ /// ```text
+ /// ┌────────────────────┐
+ /// │ Projection(t.a) │
+ /// └────────────────────┘
+ /// ▲
+ /// │
+ /// │
+ /// ┌────────────────────┐ Filter ┌────────────────────┐ Projection ┌────────────────────┐
+ /// │ Filter(t.b > 5) │────Pushdown──▶ │ Projection(t.a) │ ───Pushdown───▶ │ Projection(t.a) │
+ /// └────────────────────┘ └────────────────────┘ └────────────────────┘
+ /// ▲ ▲ ▲
+ /// │ │ │
+ /// │ │ ┌────────────────────┐
+ /// ┌────────────────────┐ ┌────────────────────┐ │ Scan │
+ /// │ Scan │ │ Scan │ │ filter=(t.b > 5) │
+ /// └────────────────────┘ │ filter=(t.b > 5) │ │ projection=(t.a) │
+ /// └────────────────────┘ └────────────────────┘
+ ///
+ /// Initial Plan If `TableProviderFilterPushDown` Projection pushdown notes that
+ /// returns true, filter pushdown the scan only needs t.a
+ /// pushes the filter into the scan
+ /// BUT internally evaluating the
+ /// predicate still requires t.b
+ /// ```
+ ///
+ /// # Limit
+ ///
+ /// If `limit` is specified, must only produce *at least* this many rows,
+ /// (though it may return more). Like Projection Pushdown and Filter
+ /// Pushdown, DataFusion pushes `LIMIT`s as far down in the plan as
+ /// possible, called "Limit Pushdown" as some sources can use this
+ /// information to improve their performance. Note that if there are any
+ /// Inexact filters pushed down, the LIMIT cannot be pushed down. This is
+ /// because inexact filters do not guarantee that every filtered row is
+ /// removed, so applying the limit could lead to too few rows being available
+ /// to return as a final result.
+ async fn scan(
+ &self,
+ state: &dyn CatalogSession,
Review Comment:
I checked in the Spice AI codebase, and this will be a relatively minor refactoring for us. I am ok with this change 👍
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2242850429
So it sounds to me like we are converging on a consensus here. I think the next steps for this PR are to
1. resolve the outstanding comments / file tickets for additional follow on work
2. Send out some announcements / discussions on the mailing list / discord / slack (I can handle this later today if no one beats me to it)
3. Wait at least 24 hours after the announcement before merging this in
But all in all I am feeling great about this one
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1687045497
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
I think prior to merging this PR we should document this trait a bit (I can help, but likely not until tomorrow)
I think it will also help people to upgrade if we add an example of how to get an actual `SessionState` reference from it (as a way to ease the migration)
```rust
let session_context = catalog_session.
.as_any()
.downcast_ref::<SessionState>()
.unwrap();
```
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248884474
> @findepi also mentioned `Session` as an option which seems reasonable to me too
I slightly prefer `SessionProvider` as I think it is more consistent with the naming conventions of other traits in DataFusion such as `TableProvider`, `CatalogProvider`, `SchemaProvider` etc, (however, there are plenty of counter examples like `ExprSchema`)
However, I don't feel super strongly and would be interested in opinions from others if we want to have a `Provider` on the end of the (now named) `Session` trait
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2250208316
rebased on current main, no other changes
i couldn't locally reproduce the build problems observed on CI
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686650408
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -49,16 +49,18 @@ impl ListingTableFactory {
impl TableProviderFactory for ListingTableFactory {
async fn create(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
- let file_format = state
+ // TODO remove downcast_ref from here. Should file format factory be an extension to session state?
Review Comment:
Good point. OTOH, `dyn TableProviderFactory` is already provided to the `ListingSchemaProvider`, so maybe the table provider could just have it, without getting it from the session?
Also, how do pluggable file format work with table formats such as Iceberg and Delta? Delta is always Parquet, but it's different Parquet metadata than Iceberg, so they are not handled the same way, are they?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2243545487
> should we proceed to the communication step?
Yes indeed -- thank you @findepi -- I posted a message in the ASF slack, discord and on the mailing list https://lists.apache.org/thread/v3g76skmwoko87xqhz8r7tjz47bhp0ts
Thank you for your help driving this project
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1687194943
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
@alamb that would help a lot. In addition this comment should be clarified on the `SessionState` struct:
```
/// Get a new TaskContext to run in this session
pub fn task_ctx(&self) -> Arc<TaskContext> {
Arc::new(TaskContext::from(self))
}
```
What I would want is to get the `TaskContext` for a specific query that was initialized by custom code, I think this will create a new one (using the `SessionState` config). My question is: how to get the `TaskContext` passed into a fn like `supports_filters_pushdown`? I am hoping with this release that this will be feasible.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2241115501
> As a follow on PR we may want to consider having `SessionCatalog` also implement `FunctionRegistry` rather than providing access to the hash maps directly
>
> https://docs.rs/datafusion/latest/datafusion/execution/trait.FunctionRegistry.html
>
> So that would mean
>
> ```rust
> pub trait CatalogSession: FunctionRegistry + Send + Sync {
> ...
> }
> ```
>
> And removing these functions
>
> However, for this PR I think keeping the API as close as possible to SessionState would be best
My concern is still there, we might need more granular trait than single `CatalogSession` for each trait
But, the suggestion above inspired me that how we could structure the trait step by step.
Given that I'm not confident about `CatalogSession` is the end state of the trait structure, we could have
`pub trait TableProviderContext: CatalogSession` for `TableProvider` to ensure we can avoid breaking the `scan` API again if we miss something. If we need a smaller trait, we just change `CatalogSession` to `SomeTrait`.
Similar idea applies to `CatalogProvider`, `SchemaProvider` and `FileFormat` if they expects SessionState in the future, avoid using the same general context in every level of trait definition helps potential circular dependency.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "phillipleblanc (via GitHub)" <gi...@apache.org>.
phillipleblanc commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2288027658
Looks like this PR didn't get tagged with `api-change`, so it got put under the "Documentation Updates" section in the v41 Changelog: https://github.com/apache/datafusion/blob/main/dev/changelog/41.0.0.md?plain=1#L94
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685368085
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -736,13 +736,16 @@ impl TableProvider for ListingTable {
async fn scan(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
projection: Option<&Vec<usize>>,
filters: &[Expr],
limit: Option<usize>,
) -> Result<Arc<dyn ExecutionPlan>> {
- let (mut partitioned_file_lists, statistics) =
- self.list_files_for_scan(state, filters, limit).await?;
+ // TODO remove downcast_ref from here?
Review Comment:
Yes, definitely we should remove the downcast (likely by adding some more functions to the `CatalogSession` trait) . But I also think we could do that as a follow on PR with minimal impact
Also this pattern shows that existing code could always use this pattern to downcast `&dy CatalogSession` to `&SessionState` which gives us an easy path for all migrations
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2241076641
> At runtime there are circular dependencies, unless we further limit amount of functionality available to the `TableProvider`. However, I think we actually solve the circular dependencies with indirection. `CatalogSession` is supposed to be a POC for that indirection.
Yes I agree with this
> Instead, I think we should provider insert_plan_to_table for SchemaProvider. And avoid insert plan inside QueryPlan but in SchemaProvider. Split the logic into two step with
I also think this sounds like a reasonable plan - and I think it becomes easier as we split the logic out and defined the API via trats
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686655186
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -49,16 +49,18 @@ impl ListingTableFactory {
impl TableProviderFactory for ListingTableFactory {
async fn create(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
- let file_format = state
+ // TODO remove downcast_ref from here. Should file format factory be an extension to session state?
Review Comment:
FWIW, `IcebergTableProvider` from https://github.com/apache/iceberg-rust
https://github.com/apache/iceberg-rust/blob/bbd042da5835b3e2e8ce7c05d45377c367464efd/crates/integrations/datafusion/src/table.rs#L76-L88 doesn't use the session at all.
i would be tempted to think that the pluggable FileFormat is equivalent to implementing Hive table format, where file format is underspecified and pluggable. Hive table format is super important industry-wide and maybe it could deserve it's own module (crate). Then, the pluggable file format functionality could be its implementation detail and not necessarily part of DF core.
@jayzhan211 @alamb please correct me if i am thinking about this in a wrong way
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685422070
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
+/// using that snapshot.
+///
+/// # Example Catalog Implementations
+///
+/// Here are some examples of how to implement custom catalogs:
+///
+/// * [`datafusion-cli`]: [`DynamicFileCatalogProvider`] catalog provider
+/// that treats files and directories on a filesystem as tables.
+///
+/// * The [`catalog.rs`]: a simple directory based catalog.
+///
+/// * [delta-rs]: [`UnityCatalogProvider`] implementation that can
+/// read from Delta Lake tables
+///
+/// [`datafusion-cli`]: https://datafusion.apache.org/user-guide/cli/index.html
+/// [`DynamicFileCatalogProvider`]: https://github.com/apache/datafusion/blob/31b9b48b08592b7d293f46e75707aad7dadd7cbc/datafusion-cli/src/catalog.rs#L75
+/// [`catalog.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/catalog.rs
+/// [delta-rs]: https://github.com/delta-io/delta-rs
+/// [`UnityCatalogProvider`]: https://github.com/delta-io/delta-rs/blob/951436ecec476ce65b5ed3b58b50fb0846ca7b91/crates/deltalake-core/src/data_catalog/unity/datafusion.rs#L111-L123
+///
+/// [`TableProvider]: crate::datasource::TableProvider
+
+pub trait CatalogProvider: Sync + Send {
+ /// Returns the catalog provider as [`Any`]
+ /// so that it can be downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;
+
+ /// Retrieves a specific schema from the catalog by name, provided it exists.
+ fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>>;
+
+ /// Adds a new schema to this catalog.
+ ///
+ /// If a schema of the same name existed before, it is replaced in
+ /// the catalog and returned.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn register_schema(
+ &self,
+ name: &str,
+ schema: Arc<dyn SchemaProvider>,
+ ) -> Result<Option<Arc<dyn SchemaProvider>>> {
+ // use variables to avoid unused variable warnings
+ let _ = name;
+ let _ = schema;
+ not_impl_err!("Registering new schemas is not supported")
+ }
+
+ /// Removes a schema from this catalog. Implementations of this method should return
+ /// errors if the schema exists but cannot be dropped. For example, in DataFusion's
+ /// default in-memory catalog, `MemoryCatalogProvider`, a non-empty schema
+ /// will only be successfully dropped when `cascade` is true.
+ /// This is equivalent to how DROP SCHEMA works in PostgreSQL.
+ ///
+ /// Implementations of this method should return None if schema with `name`
+ /// does not exist.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn deregister_schema(
+ &self,
+ _name: &str,
Review Comment:
like 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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2238975365
At runtime there are circular dependencies, unless we further limit amount of functionality available to the `TableProvider`.
However, I think we actually solve the circular dependencies with indirection. `CatalogSession` is supposed to be a POC for that indirection.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2241168485
This really is a clever way to resolve this, and it will definitely help with #11193 . As @jayzhan211 mentions it will have breaking API changes to a commonly used API (`TableProvider`), well worth it IMO. I would recommend making this as complete as possible to avoid future API changes.
Great work, these are tricky things to refactor.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686728296
##########
Cargo.toml:
##########
@@ -20,6 +20,7 @@ exclude = ["datafusion-cli", "dev/depcheck"]
members = [
"datafusion/common",
"datafusion/common-runtime",
+ "datafusion/catalog",
Review Comment:
pushed a change for this (https://github.com/apache/datafusion/pull/11516#issuecomment-2243214905)
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1690363719
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
Added a new commit with some docs.
Please help me improve them, it doesn't feel complete yet.
> I think it will also help people to upgrade if we add an example of how to get an actual `SessionState` reference from it (as a way to ease the migration)
>
> ```rust
> let session_context = catalog_session.
> .as_any()
> .downcast_ref::<SessionState>()
> .unwrap();
> ```
i like the idea.
I added this as preformatted text, but it should be a code snippet that `cargo doc` can check for compilation.
How should i handle this without introducing circular dependency between the new catalog module and the core?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248819393
@findepi also mentioned `Session` as an option which seems reasonable to me too
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2250753244
> rebased on current main, no other changes i couldn't locally reproduce the build problems observed on CI
I think the failure is due to new rust version (if you run `rustup update` it would likely start failing too).
https://github.com/apache/datafusion/issues/11651
Not related to the changes in this PR
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2251415700
> I think the failure is due to new rust version (if you run `rustup update` it would likely start failing too).
>
> #11651
it's now fixed and the build is green, but there is a new conflict. will rebase
--
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] Extract catalog API to separate crate [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2233446186
This tries to implement the idea expressed in https://github.com/apache/datafusion/issues/10782#issuecomment-2233112949
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2249795370
> Oh no, I broke the doc test -- sorry @findepi
Here is a proposed fix: https://github.com/findepi/datafusion/pull/5
--
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] Extract catalog API to separate crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2238946024
Given the current `FileFormatFactory` implementation in datafusion, we only need `TableOptions`, we can change SessionState to `FileFormatContext ` with `TableOptions`, and it can move out from the crate which seat `Sessionstate`.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686626658
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
+/// using that snapshot.
+///
+/// # Example Catalog Implementations
+///
+/// Here are some examples of how to implement custom catalogs:
+///
+/// * [`datafusion-cli`]: [`DynamicFileCatalogProvider`] catalog provider
+/// that treats files and directories on a filesystem as tables.
+///
+/// * The [`catalog.rs`]: a simple directory based catalog.
+///
+/// * [delta-rs]: [`UnityCatalogProvider`] implementation that can
+/// read from Delta Lake tables
+///
+/// [`datafusion-cli`]: https://datafusion.apache.org/user-guide/cli/index.html
+/// [`DynamicFileCatalogProvider`]: https://github.com/apache/datafusion/blob/31b9b48b08592b7d293f46e75707aad7dadd7cbc/datafusion-cli/src/catalog.rs#L75
+/// [`catalog.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/catalog.rs
+/// [delta-rs]: https://github.com/delta-io/delta-rs
+/// [`UnityCatalogProvider`]: https://github.com/delta-io/delta-rs/blob/951436ecec476ce65b5ed3b58b50fb0846ca7b91/crates/deltalake-core/src/data_catalog/unity/datafusion.rs#L111-L123
+///
+/// [`TableProvider]: crate::datasource::TableProvider
+
+pub trait CatalogProvider: Sync + Send {
+ /// Returns the catalog provider as [`Any`]
+ /// so that it can be downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;
+
+ /// Retrieves a specific schema from the catalog by name, provided it exists.
+ fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>>;
+
+ /// Adds a new schema to this catalog.
+ ///
+ /// If a schema of the same name existed before, it is replaced in
+ /// the catalog and returned.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn register_schema(
+ &self,
+ name: &str,
Review Comment:
Good catch! in this PR there is a lot of code being moved around. i tried to minimize the edits when doing moves, since it's hard to later find them in the git history. Let's address this as a follow-up, unless we rebase-merge PRs, then I am happy to add more changes within the PR.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686742327
##########
datafusion/catalog/src/catalog.rs:
##########
@@ -0,0 +1,173 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::any::Any;
+use std::sync::Arc;
+
+pub use crate::schema::SchemaProvider;
+use datafusion_common::not_impl_err;
+use datafusion_common::Result;
+
+/// Represents a catalog, comprising a number of named schemas.
+///
+/// # Catalog Overview
+///
+/// To plan and execute queries, DataFusion needs a "Catalog" that provides
+/// metadata such as which schemas and tables exist, their columns and data
+/// types, and how to access the data.
+///
+/// The Catalog API consists:
+/// * [`CatalogProviderList`]: a collection of `CatalogProvider`s
+/// * [`CatalogProvider`]: a collection of `SchemaProvider`s (sometimes called a "database" in other systems)
+/// * [`SchemaProvider`]: a collection of `TableProvider`s (often called a "schema" in other systems)
+/// * [`TableProvider]`: individual tables
+///
+/// # Implementing Catalogs
+///
+/// To implement a catalog, you implement at least one of the [`CatalogProviderList`],
+/// [`CatalogProvider`] and [`SchemaProvider`] traits and register them
+/// appropriately in the `SessionContext`.
+///
+/// DataFusion comes with a simple in-memory catalog implementation,
+/// `MemoryCatalogProvider`, that is used by default and has no persistence.
+/// DataFusion does not include more complex Catalog implementations because
+/// catalog management is a key design choice for most data systems, and thus
+/// it is unlikely that any general-purpose catalog implementation will work
+/// well across many use cases.
+///
+/// # Implementing "Remote" catalogs
+///
+/// Sometimes catalog information is stored remotely and requires a network call
+/// to retrieve. For example, the [Delta Lake] table format stores table
+/// metadata in files on S3 that must be first downloaded to discover what
+/// schemas and tables exist.
+///
+/// [Delta Lake]: https://delta.io/
+///
+/// The [`CatalogProvider`] can support this use case, but it takes some care.
+/// The planning APIs in DataFusion are not `async` and thus network IO can not
+/// be performed "lazily" / "on demand" during query planning. The rationale for
+/// this design is that using remote procedure calls for all catalog accesses
+/// required for query planning would likely result in multiple network calls
+/// per plan, resulting in very poor planning performance.
+///
+/// To implement [`CatalogProvider`] and [`SchemaProvider`] for remote catalogs,
+/// you need to provide an in memory snapshot of the required metadata. Most
+/// systems typically either already have this information cached locally or can
+/// batch access to the remote catalog to retrieve multiple schemas and tables
+/// in a single network call.
+///
+/// Note that [`SchemaProvider::table`] is an `async` function in order to
+/// simplify implementing simple [`SchemaProvider`]s. For many table formats it
+/// is easy to list all available tables but there is additional non trivial
+/// access required to read table details (e.g. statistics).
+///
+/// The pattern that DataFusion itself uses to plan SQL queries is to walk over
+/// the query to find all table references,
+/// performing required remote catalog in parallel, and then plans the query
+/// using that snapshot.
+///
+/// # Example Catalog Implementations
+///
+/// Here are some examples of how to implement custom catalogs:
+///
+/// * [`datafusion-cli`]: [`DynamicFileCatalogProvider`] catalog provider
+/// that treats files and directories on a filesystem as tables.
+///
+/// * The [`catalog.rs`]: a simple directory based catalog.
+///
+/// * [delta-rs]: [`UnityCatalogProvider`] implementation that can
+/// read from Delta Lake tables
+///
+/// [`datafusion-cli`]: https://datafusion.apache.org/user-guide/cli/index.html
+/// [`DynamicFileCatalogProvider`]: https://github.com/apache/datafusion/blob/31b9b48b08592b7d293f46e75707aad7dadd7cbc/datafusion-cli/src/catalog.rs#L75
+/// [`catalog.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/catalog.rs
+/// [delta-rs]: https://github.com/delta-io/delta-rs
+/// [`UnityCatalogProvider`]: https://github.com/delta-io/delta-rs/blob/951436ecec476ce65b5ed3b58b50fb0846ca7b91/crates/deltalake-core/src/data_catalog/unity/datafusion.rs#L111-L123
+///
+/// [`TableProvider]: crate::datasource::TableProvider
+
+pub trait CatalogProvider: Sync + Send {
+ /// Returns the catalog provider as [`Any`]
+ /// so that it can be downcast to a specific implementation.
+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;
+
+ /// Retrieves a specific schema from the catalog by name, provided it exists.
+ fn schema(&self, name: &str) -> Option<Arc<dyn SchemaProvider>>;
+
+ /// Adds a new schema to this catalog.
+ ///
+ /// If a schema of the same name existed before, it is replaced in
+ /// the catalog and returned.
+ ///
+ /// By default returns a "Not Implemented" error
+ fn register_schema(
+ &self,
+ name: &str,
Review Comment:
Does this apply to parameter names in traits too? or only in impls?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2243245595
i went through the PR conversations and marked as resolved those resolved ones. I don't see any outstanding items, but correct me please if i missed something.
@jayzhan211 provided action items in https://github.com/apache/datafusion/pull/11516#pullrequestreview-2189976292
the one about catalog names is applied, the one about separate traits is replied to (https://github.com/apache/datafusion/pull/11516#issuecomment-2243062441) and it looks we're good here.
@alamb posted next steps in https://github.com/apache/datafusion/pull/11516#pullrequestreview-2189874419
should we proceed to the communication step?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685421792
##########
datafusion-cli/src/catalog.rs:
##########
@@ -237,7 +236,7 @@ fn substitute_tilde(cur: String) -> String {
mod tests {
use super::*;
- use datafusion::catalog::schema::SchemaProvider;
+ use datafusion::catalog_api::SchemaProvider;
Review Comment:
why does it called catalog-api but not the same name as crate?
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1686600004
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
The parts which are not core-dependent are small.
Please also check `downcast_ref::<SessionState>()` added in this PR, as some usages are currently hidden via Any. Some of them may get promoted to CatalogSession, some perhaps won't be.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1685423286
##########
datafusion/core/src/datasource/listing_table_factory.rs:
##########
@@ -49,16 +49,18 @@ impl ListingTableFactory {
impl TableProviderFactory for ListingTableFactory {
async fn create(
&self,
- state: &SessionState,
+ state: &dyn CatalogSession,
cmd: &CreateExternalTable,
) -> Result<Arc<dyn TableProvider>> {
- let file_format = state
+ // TODO remove downcast_ref from here. Should file format factory be an extension to session state?
Review Comment:
Not sure what do you mean by this 😖
I think the role of FileFormat to SessionState is similar to function to SessionState, they are already registrable
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2265409603
> @berkaysynnada yes, #11600
I suggest adding a comment in the code with the link to the ticket to help others find this ticket
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2244076648
@findepi
I see your PR plan to close #10782, isn't it the first step(? Given I didn't see `datafusion-catalog-common`, I guess there is some misunderstand about the next steps of #10782
I think the TODOs for #10782
1. Move trait implementation to `datafusion-catalog` crate https://github.com/apache/datafusion/issues/11182
* TableProvider
* CatalogProvider
* SchemaProvider
* MemoryCatalogProvider
* InformationSchemaProvider
* DynamicFileCatalog
2. (eventually) the LIstingTableProvider (which is by far the most complicated) moved to its own crate `datafusion-catalog-listing`
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1690364329
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
I have some thoughts -- let me give it a try.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2252947090
Let's do it. go go go 🚀
Thanks again everryone for your comments and help. I think this PR finally breaks open the path to separate out the last monolithic knot
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2252949921
🎉 thanks for all the review feedback and the merge!
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #11516:
URL: https://github.com/apache/datafusion/pull/11516
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#discussion_r1690425053
##########
datafusion/catalog/src/session.rs:
##########
@@ -0,0 +1,102 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use async_trait::async_trait;
+use datafusion_common::config::ConfigOptions;
+use datafusion_common::{DFSchema, Result};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::{AggregateUDF, Expr, LogicalPlan, ScalarUDF, WindowUDF};
+use datafusion_physical_plan::{ExecutionPlan, PhysicalExpr};
+use std::any::Any;
+use std::collections::HashMap;
+use std::sync::Arc;
+
+#[async_trait]
+pub trait CatalogSession: Send + Sync {
Review Comment:
thank you, that's well written!
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248899510
re: SessionProvider
the trait describes something that doesn't _provide session_, it describes something that _is session_.
However, same think can be said about TableProvider or CatalogProvider. I don't have an answer to the consistency argument...
i can change, this is not a big deal. @alamb @jayzhan211 @cisaacson @phillipleblanc @Omega359 let's just make sure we have agreement to avoid unnecessary back and forth.
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2248878418
I am feeling even better about this PR. I propose we plan to merge it in 2 days time (Friday) unless there are objections or other comments to address. Thank you @findepi @Omega359 @cisaacson @jayzhan211 and @phillipleblanc for your help so far
--
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] Extract catalog API to separate crate, change `TableProvider::scan` to take a trait rather than `SessionState` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #11516:
URL: https://github.com/apache/datafusion/pull/11516#issuecomment-2265643243
good idea! https://github.com/apache/datafusion/pull/11784
--
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