alamb commented on code in PR #4607:
URL: https://github.com/apache/arrow-datafusion/pull/4607#discussion_r1061894199
##########
datafusion/core/tests/sqllogictests/test_files/select.slt:
##########
@@ -21,9 +21,8 @@
##########
# all where empty
-query II
+statement ok
Review Comment:
I don't understand the need for any of the changes in this file
##########
datafusion/core/src/execution/context.rs:
##########
@@ -239,20 +241,8 @@ impl SessionContext {
/// This method is `async` because queries of type `CREATE EXTERNAL TABLE`
Review Comment:
I wonder if this docstring needs to be updated
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1641,6 +1619,97 @@ impl SessionState {
self
}
+ /// 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> {
+ use crate::catalog::information_schema::INFORMATION_SCHEMA_TABLES;
+ use datafusion_sql::parser::Statement as DFStatement;
+ use sqlparser::ast::*;
+ use std::collections::hash_map::Entry;
+
+ let mut statements = DFParser::parse_sql(sql)?;
+ if statements.len() != 1 {
+ return Err(DataFusionError::NotImplemented(
+ "The context currently only supports a single SQL
statement".to_string(),
+ ));
+ }
+ let statement = statements.pop_front().unwrap();
+
+ let mut relations = hashbrown::HashSet::with_capacity(10);
Review Comment:
```suggestion
// Getting `TableProviders` is async but planing is not -- thus
pre-fetch table providers for all
// relations referenced in this query
let mut relations = hashbrown::HashSet::with_capacity(10);
```
##########
datafusion/core/src/catalog/schema.rs:
##########
@@ -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>>;
Review Comment:
cc @avantgardnerio
##########
datafusion/core/src/execution/context.rs:
##########
@@ -239,20 +241,8 @@ impl SessionContext {
/// This method is `async` because queries of type `CREATE EXTERNAL TABLE`
Review Comment:
I also recommend adding a note like:
```suggestion
/// This method is `async` because queries of type `CREATE EXTERNAL
TABLE`
///
/// Note: This api implements DDL such as `CREATE TABLE` and `CREATE
VIEW` with in memory
/// default implementations. Please
[`SessionState::create_logical_plan()`] which does not
/// mutate the state based on such statements.
```
##########
datafusion/core/tests/sql/errors.rs:
##########
@@ -132,41 +132,3 @@ async fn invalid_qualified_table_references() ->
Result<()> {
}
Ok(())
}
-
-#[tokio::test]
Review Comment:
I would still like this test to be present (updated to call
`SessionState::create_logical_plan`)
I think it is important to have this test because it communicates the need /
intent for the "don't mutate session state" APIs
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
