You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "epgif (via GitHub)" <gi...@apache.org> on 2025/06/13 15:51:51 UTC
[PR] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
epgif opened a new pull request, #16401:
URL: https://github.com/apache/datafusion/pull/16401
InformationSchemaConfig::make_tables only needs the TableType not the whole TableProvider, and the former may require an expensive catalog operation to construct and the latter may not.
This allows avoiding `SELECT * FROM information_schema.tables` having to make 1 of those potentially expensive operations per table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2153457367
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
Sorry, messed with another repo. in this file it should be like `datafusion_common::Result`
in DF we have a type alias in `common` crate
```
pub type Result<T, E = DataFusionError> = result::Result<T, E>;
```
please reuse it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2147428867
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
```suggestion
async fn table_type(&self, name: &str) -> DataFusionResult<Option<TableType>> {
```
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#issuecomment-2982140980
> @alamb
>
> > I wonder if there is some way we can write a test for it (mostly to prevent it from being accidentally broken/changed in the future)
>
> I looked around for some tests implementing this interface that I could extend, but didn't find any.
I tried panic-driven development and identified various unrelated test, none providing a clue.
The trick here is a test needs to check that one function is called rather than another. This seems like a classic low-level unit test.
So, that's what I've done.
It looks noisy if you're not used to this kind of test, but it does work, and does provide the expected failure when the `information_schema.rs` part is reverted (i.e. wrong function called).
Let me know what you think.
Thanks, @alamb!
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#issuecomment-2971013443
@alamb
> I wonder if there is some way we can write a test for it (mostly to prevent it from being accidentally broken/changed in the future)
I looked around for some tests implementing this interface that I could extend, but didn't find any. I meant to ask earlier but forgot: is there anything like that? It would be a big help if I had some previous tests to take inspiration from.
Failing that, I'll take a stab at it anyway :)
Thanks!
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2151033231
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
+ self.table(name).await.map(|o| o.map(|t| t.table_type()))
Review Comment:
Changed to `and_then`.
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2153345514
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
+ self.table(name).await.map(|o| o.map(|t| t.table_type()))
Review Comment:
Well, clippy suggested what I had to begin with:
```
warning: using `Result.and_then(|x| Ok(y))`, which is more succinctly expressed as `map(|x| y)`
--> datafusion/catalog/src/schema.rs:63:9
|
63 | / self.table(name)
64 | | .await
65 | | .and_then(|o| Ok(o.map(|t| t.table_type())))
| |________________________________________________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bind_instead_of_map
= note: `#[warn(clippy::bind_instead_of_map)]` on by default
help: try
|
63 ~ self.table(name)
64 + .await.map(|o| o.map(|t| t.table_type()))
```
So I changed it back.
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2147429310
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
+ self.table(name).await.map(|o| o.map(|t| t.table_type()))
Review Comment:
is there any way to avoid nested `map`? perhaps `flat_map` or `and_then` ? would me more readable IMHO
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2154936751
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
Please change also one in the test ```
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError> {
```
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
Please change also one in the test
```
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError> {
```
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2155260272
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
All references in the test are also already `datafusion_common::Result`, thanks to `use super::*`. However, as before, this one can be simplified as `DataFusionError` is already the default. The only reason the code I added is like that is it's copied and pasted from what is already here.
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #16401:
URL: https://github.com/apache/datafusion/pull/16401
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2154521172
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
All references to `Result` in this file are already the one from `datafusion_common`, including what I've added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2151032750
##########
datafusion/catalog/src/schema.rs:
##########
@@ -54,6 +55,14 @@ pub trait SchemaProvider: Debug + Sync + Send {
name: &str,
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError>;
+ /// Retrieves the type of a specific table from the schema by name, if it exists, otherwise
+ /// returns `None`. Implementations for which this operation is cheap but [Self::table] is
+ /// expensive can override this to improve operations that only need the type, e.g.
+ /// `SELECT * FROM information_schema.tables`.
+ async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> {
Review Comment:
I don't see any `DataFusionResult` in the project, but I do see that `E = DataFusionError` is the default so I dropped that redundancy. Is that what you meant?
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "epgif (via GitHub)" <gi...@apache.org>.
epgif commented on PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#issuecomment-2978427374
Please take another look @comphead -- thanks!
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#issuecomment-2985697551
Thanks again @epgif and @comphead
--
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] feat: add SchemaProvider::table_type(table_name: &str) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #16401:
URL: https://github.com/apache/datafusion/pull/16401#discussion_r2155411540
##########
datafusion/catalog/src/information_schema/tests.rs:
##########
@@ -0,0 +1,88 @@
+use std::sync::Arc;
Review Comment:
The CI is failing because this file doesn't have the ASF header
- Actually, it seems we were checking twice 🤦 https://github.com/apache/datafusion/pull/16451
I think it is more common that these types of tests go in the same module (file) at the bottom in the DataFusion codebase anyways so I'll move them there to fix the CI as well
I'll move them there
--
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