feat: add SchemaProvider::table_type(table_name: &str) #16401
Conversation
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! |
datafusion/catalog/src/schema.rs
Outdated
| /// 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> { |
There was a problem hiding this comment.
| async fn table_type(&self, name: &str) -> Result<Option<TableType>, DataFusionError> { | |
| async fn table_type(&self, name: &str) -> DataFusionResult<Option<TableType>> { |
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this comment.
All references to Result in this file are already the one from datafusion_common, including what I've added.
There was a problem hiding this comment.
Please change also one in the test
) -> Result<Option<Arc<dyn TableProvider>>, DataFusionError> {
There was a problem hiding this 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.
| /// 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())) |
There was a problem hiding this comment.
is there any way to avoid nested map? perhaps flat_map or and_then ? would me more readable IMHO
There was a problem hiding this 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.
epgif
force-pushed
the
schema-provider-table-type
branch
from
50e9552 to
6961bcf
Compare
|
Please take another look @comphead -- thanks! |
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.
epgif
force-pushed
the
schema-provider-table-type
branch
from
6961bcf to
9366395
Compare
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 Let me know what you think. Thanks, @alamb! |
epgif
force-pushed
the
schema-provider-table-type
branch
from
9366395 to
50e2ecb
Compare
| @@ -0,0 +1,88 @@ | |||
| use std::sync::Arc; | |||
There was a problem hiding this comment.
The CI is failing because this file doesn't have the ASF header
- Actually, it seems we were checking twice 🤦 Remove redundant license-header-check CI job #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
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.tableshaving to make 1 of those potentially expensive operations per table.