Make SchemaProvider::table async #4607
Conversation
github-actions
bot
added
core
| @@ -35,7 +37,7 @@ pub trait SchemaProvider: Sync + Send { | |||
| fn table_names(&self) -> Vec<String>; | |||
|
|
|||
| /// Retrieves a specific table from the schema by name, provided it exists. | |||
| fn table(&self, name: &str) -> Option<Arc<dyn TableProvider>>; | |||
| async fn table(&self, name: &str) -> Option<Arc<dyn TableProvider>>; | |||
There was a problem hiding this comment.
And this is the change to support async catalogs
tustvold
force-pushed
the
async-catalog
branch
from
a3d3f3a
to
8c0d1ca
Compare
|
This is temporarily on hold pending the work I am doing on cleaning up the config, see #4617. I will come back to this once that is complete |
tustvold
force-pushed
the
async-catalog
branch
from
9a4c216
to
cf10913
Compare
tustvold
force-pushed
the
async-catalog
branch
from
cf10913
to
6482032
Compare
|
I think this is now ready for review, it would be amazing if this could make this weeks release as this has been a frequently requested feature |
| /// Creates a [`LogicalPlan`] from the provided SQL string | ||
| /// | ||
| /// See [`SessionContext::sql`] for a higher-level interface that also handles DDL | ||
| pub async fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> { |
There was a problem hiding this comment.
This is specifically broken out into a separate function so that database's that want to not handle DDL, such as IOx, don't have to re-implement this functionality. FYI @alamb
| } | ||
|
|
||
| // Always include information_schema if available | ||
| if self.config.information_schema() { |
There was a problem hiding this comment.
#4606 means there isn't a cost to doing this
|
Will review this tomorrow. Note the CI is failing |
| ControlFlow::Continue(()) | ||
| } | ||
|
|
||
| fn pre_visit_statement( |
There was a problem hiding this comment.
This is a bit gross, but I hope to split apart the query planning from the other types of query, that will clean this up.
This change makes sense to me. It's a good solution to only change one interface, I like it! The only thing left is asynchronous register things, but on the one hand it's less important than getting and deregistering, and another hand there are still ways to workaround (like update-on-getting). So I'm +1 for providing this change as-is. Thanks for making this @tustvold 🥳 |
We can easily add this as a follow up, as the catalog is largely a detail of SessionContext, I don't imagine it causing any major issues. |
| @@ -35,7 +37,7 @@ pub trait SchemaProvider: Sync + Send { | |||
| fn table_names(&self) -> Vec<String>; | |||
|
|
|||
| /// Retrieves a specific table from the schema by name, provided it exists. | |||
| fn table(&self, name: &str) -> Option<Arc<dyn TableProvider>>; | |||
| async fn table(&self, name: &str) -> Option<Arc<dyn TableProvider>>; | |||
|
Benchmark runs are scheduled for baseline = 087ac09 and contender = fad77a4. fad77a4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3777.
Rationale for this change
What changes are included in this PR?
Makes
SchemaProvider::tableasync.Are these changes tested?
Are there any user-facing changes?