You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/12/23 19:32:27 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #4721: Non-deprecated support for planning SQL without DDL
alamb opened a new pull request, #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721
# Which issue does this PR close?
Closes https://github.com/apache/arrow-datafusion/issues/4720
# Rationale for this change
More details on ticket https://github.com/apache/arrow-datafusion/issues/4720
# What changes are included in this PR?
1. Change APIs to allow query planning without DDL execution (TODO)
2. Add doc comments about the usecase
2. Update errors in physical planner to return DataFusion::Error someone attempts to plan a DDL it is unsupported
# Are these changes tested?
Yes, existing tests
# Are there any user-facing changes?
1. API is slightly different (different names)
1. Error messages are more precise
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056812617
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1097,15 +1097,15 @@ impl DefaultPhysicalPlanner {
// TABLE" -- it must be handled at a higher level (so
// that the appropriate table can be registered with
// the context)
- Err(DataFusionError::Internal(
+ Err(DataFusionError::Plan(
Review Comment:
https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1364522866 are the key usecases -- as long as they are possible / easy / well documented it will be great
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1364523557
I will work on this after christmas (27th). I think the key thing is to not be mixing high-level APIs i.e. SessionContext with low-level concepts LogicalPlan. The key thing is to ensure that planning, optimisation and execution take place against the same SessionState. The interior mutability of SessionContext makes this impossible
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb closed pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
URL: https://github.com/apache/arrow-datafusion/pull/4721
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1365841477
POC in https://github.com/influxdata/influxdb_iox/pull/6469 - the TLDR is to use SessionState directly
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056812534
##########
datafusion/core/src/execution/context.rs:
##########
@@ -559,11 +573,9 @@ impl SessionContext {
}
Ok(false)
}
- /// Creates a logical plan.
- ///
- /// This function is intended for internal use and should not be called directly.
- #[deprecated(note = "Use SessionContext::sql which snapshots the SessionState")]
- pub fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
+
+ /// Creates a [`LogicalPlan`] from a SQL query.
+ pub fn plan_sql(&self, sql: &str) -> Result<LogicalPlan> {
Review Comment:
> Again this appears to just call DFParser followed by the query planner
Yes that is exactly what it does.
There needs to be some way for users to create a LogicalPlan and get datafusion to optimize and run it properly it (e.g. if the user makes the LogicalPlan directly from their own query language such as influxrpc or VegaFusion)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056643369
##########
datafusion/core/src/execution/context.rs:
##########
@@ -492,6 +497,15 @@ impl SessionContext {
}
}
+ /// Creates a [`DataFrame`] that will execute the specified
+ /// LogicalPlan, but will error if the plans represent DDL such as
+ /// `CREATE TABLE`
+ ///
+ /// Use [`Self::dataframe`] to run plans with DDL
+ pub fn dataframe_without_ddl(&self, plan: LogicalPlan) -> Result<DataFrame> {
Review Comment:
This is the API to support https://github.com/apache/arrow-datafusion/issues/4720
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1097,15 +1097,15 @@ impl DefaultPhysicalPlanner {
// TABLE" -- it must be handled at a higher level (so
// that the appropriate table can be registered with
// the context)
- Err(DataFusionError::Internal(
+ Err(DataFusionError::Plan(
Review Comment:
These are not really "internal errors" as they can be triggered by trying to run a sql query that contains DDL
##########
datafusion/core/src/execution/context.rs:
##########
@@ -245,26 +245,31 @@ impl SessionContext {
self.state.read().config.clone()
}
- /// Creates a [`DataFrame`] that will execute a SQL query.
+ /// Creates a [`DataFrame`] that executes a SQL query supported by
+ /// DataFusion, including DDL such as (such as `CREATE TABLE`).
///
- /// This method is `async` because queries of type `CREATE EXTERNAL TABLE`
- /// might require the schema to be inferred.
+ /// You can use [`Self::plan_sql`] and
+ /// [`DataFrame::create_physical_plan`] directly if you need read only
+ /// query support (no way to create external tables, for example)
+ ///
+ /// This method is `async` because queries of type `CREATE
+ /// EXTERNAL TABLE` might require the schema to be inferred.
pub async fn sql(&self, sql: &str) -> Result<DataFrame> {
- 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(),
- ));
- }
-
- // create a query planner
- let plan = {
- // TODO: Move catalog off SessionState onto SessionContext
- let state = self.state.read();
- let query_planner = SqlToRel::new(&*state);
- query_planner.statement_to_plan(statements.pop_front().unwrap())?
- };
+ let plan = self.plan_sql(sql)?;
+ self.dataframe(plan).await
+ }
+ /// Creates a [`DataFrame`] that will execute the specified
+ /// LogicalPlan, including DDL such as (such as `CREATE TABLE`).
Review Comment:
```suggestion
/// LogicalPlan, including DDL (such as `CREATE TABLE`).
```
##########
datafusion/core/src/execution/context.rs:
##########
@@ -245,26 +245,31 @@ impl SessionContext {
self.state.read().config.clone()
}
- /// Creates a [`DataFrame`] that will execute a SQL query.
+ /// Creates a [`DataFrame`] that executes a SQL query supported by
+ /// DataFusion, including DDL such as (such as `CREATE TABLE`).
///
- /// This method is `async` because queries of type `CREATE EXTERNAL TABLE`
- /// might require the schema to be inferred.
+ /// You can use [`Self::plan_sql`] and
+ /// [`DataFrame::create_physical_plan`] directly if you need read only
+ /// query support (no way to create external tables, for example)
+ ///
+ /// This method is `async` because queries of type `CREATE
+ /// EXTERNAL TABLE` might require the schema to be inferred.
pub async fn sql(&self, sql: &str) -> Result<DataFrame> {
Review Comment:
the core SessionContext::sql API does not change
##########
benchmarks/src/bin/tpch.rs:
##########
@@ -324,18 +324,17 @@ async fn execute_query(
debug: bool,
enable_scheduler: bool,
) -> Result<Vec<RecordBatch>> {
- let plan = ctx.sql(sql).await?;
- let plan = plan.into_unoptimized_plan();
+ let plan = ctx.sql(sql).await?.into_unoptimized_plan();
if debug {
println!("=== Logical plan ===\n{:?}\n", plan);
}
- let plan = ctx.optimize(&plan)?;
+ let plan = ctx.dataframe(plan).await?.into_optimized_plan()?;
Review Comment:
this is the new pattern to get an optimized plan (rather than calling `ctx.optimize` directly
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056695809
##########
datafusion/core/src/execution/context.rs:
##########
@@ -559,11 +573,9 @@ impl SessionContext {
}
Ok(false)
}
- /// Creates a logical plan.
- ///
- /// This function is intended for internal use and should not be called directly.
- #[deprecated(note = "Use SessionContext::sql which snapshots the SessionState")]
- pub fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
+
+ /// Creates a [`LogicalPlan`] from a SQL query.
+ pub fn plan_sql(&self, sql: &str) -> Result<LogicalPlan> {
Review Comment:
It is also problematic for the same reason that create_logical_plan is problematic - it returns a LogicalPlan without any mechanism to optimise/execute against the same state
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1364522866
I think some key capabilities for all users of DataFusion (including IOx) are:
1. Create a LogicalPlan from some source (e.g your own frontend) and have datafusion plan / run that for you
2. Read only SQL (no DDL / session state modifications via SQL)
As long as those are possible and well documented I do not have strong opinions on the API
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1364523321
Marking as a Draft as I think @tustvold plans an alternate proposal and we can continue to us the deprecated APIs in IOx for the time being
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#issuecomment-1366613113
This PR has been rendered obsolete via https://github.com/apache/arrow-datafusion/pull/4750
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056685904
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1097,15 +1097,15 @@ impl DefaultPhysicalPlanner {
// TABLE" -- it must be handled at a higher level (so
// that the appropriate table can be registered with
// the context)
- Err(DataFusionError::Internal(
+ Err(DataFusionError::Plan(
Review Comment:
Imo this is a footgun we should aim to remove, I have a plan
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056684089
##########
datafusion/core/src/execution/context.rs:
##########
@@ -559,11 +573,9 @@ impl SessionContext {
}
Ok(false)
}
- /// Creates a logical plan.
- ///
- /// This function is intended for internal use and should not be called directly.
- #[deprecated(note = "Use SessionContext::sql which snapshots the SessionState")]
- pub fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
+
+ /// Creates a [`LogicalPlan`] from a SQL query.
+ pub fn plan_sql(&self, sql: &str) -> Result<LogicalPlan> {
Review Comment:
Again this appears to just call DFParser followed by the query planner
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056683926
##########
datafusion/core/src/execution/context.rs:
##########
@@ -492,6 +497,15 @@ impl SessionContext {
}
}
+ /// Creates a [`DataFrame`] that will execute the specified
+ /// LogicalPlan, but will error if the plans represent DDL such as
+ /// `CREATE TABLE`
+ ///
+ /// Use [`Self::dataframe`] to run plans with DDL
+ pub fn dataframe_without_ddl(&self, plan: LogicalPlan) -> Result<DataFrame> {
Review Comment:
This appears to just be DataFrame new, do we really need 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #4721: Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #4721:
URL: https://github.com/apache/arrow-datafusion/pull/4721#discussion_r1056685904
##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -1097,15 +1097,15 @@ impl DefaultPhysicalPlanner {
// TABLE" -- it must be handled at a higher level (so
// that the appropriate table can be registered with
// the context)
- Err(DataFusionError::Internal(
+ Err(DataFusionError::Plan(
Review Comment:
Imo this is a footgun we should aim to remove
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org