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 2023/01/17 23:06:46 UTC
[GitHub] [arrow-datafusion] avantgardnerio opened a new pull request, #4958: Split function
avantgardnerio opened a new pull request, #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958
# Which issue does this PR close?
Closes #4957.
# Rationale for this change
Described in issue.
# What changes are included in this PR?
Splitting planning and parsing.
# Are these changes tested?
Transitively, yes.
# Are there any user-facing changes?
New API methods, no breaking changes.
--
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] gruuya commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
gruuya commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1073510915
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
It may be too late by now, but I have a slightly different proposal for this issue (thanks for addressing it btw!).
Namely, while this solution handles the stated case well it doesn't address the general problem of `SqlToRel` now being hard/impossible to instantiate properly (with the new trait bound for `ContextProvider`, which is only implemented for a private struct, and the entire `information_schema` module also being private further complicating a custom implementation of `ContextProvider`). This in turn forbids usage of other useful public `SqlToRel` methods, such as `sql_to_expr`.
My proposal would be to simply rename `statement_to_plan` to `statement_to_planner`, and then return `SqlToRel::new(&provider)` at line 1740. Subsequently, to address the original issue just revise this part:
```suggestion
pub async fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
let statement = self.sql_to_statement(sql)?;
let planner = self.statement_to_planner(statement).await?;
let plan = planner.statement_to_plan(statement);
Ok(plan)
}
```
while anyone else can then get easy access to the 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] alamb commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1073840524
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
> Namely, while this solution handles the stated case well it doesn't address the general problem of SqlToRel now being hard/impossible to instantiate properly (with the new trait bound for ContextProvider, which is only implemented for a private struct, and the entire information_schema module also being private further complicating a custom implementation of ContextProvider). This in turn forbids usage of other useful public SqlToRel methods, such as sql_to_expr.
@gruuya I don't think we have contemplated using `SqlToRel` directly in the past. If that is a usecase that is important, what I found works well is to make an example demonstrating what you are doing. Here are some examples:
* https://github.com/apache/arrow-datafusion/blob/master/datafusion-examples/examples/rewrite_expr.rs
* https://github.com/apache/arrow-datafusion/blob/master/datafusion-examples/examples/custom_datasource.rs
An example has several advantages:
1. It ensures your usecase is clearly documented so people who are changing DataFusion know it is used and don't break it accidentally
3. It then gives you an example to start from if the interface changes (you can look at the example to see how the interface may have changed)
4. It helps others who might have the same need
--
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] avantgardnerio commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1073789901
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1636,22 +1636,34 @@ 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;
-
+ /// Convert a sql string into an ast Statement
+ pub fn sql_to_statement(
+ &self,
+ sql: &str,
+ ) -> Result<datafusion_sql::parser::Statement> {
let mut statements = DFParser::parse_sql(sql)?;
- if statements.len() != 1 {
+ 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 statement = statements.pop_front().ok_or_else(|| {
+ DataFusionError::NotImplemented(
+ "The context requires a statement!".to_string(),
+ )
+ })?;
+ Ok(statement)
+ }
+
+ /// Convert an ast Statement into a LogicalPlan
+ pub async fn statement_to_plan(
+ &self,
+ statement: datafusion_sql::parser::Statement,
+ ) -> 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;
Review Comment:
Agreed? They just "came along for the ride" following the "do no harm" principle. That's not a great excuse though.
I think they are in the function because this file was becoming very long, and I suspect collisions with `parser::Statement` and `ast::Statement` and probably others was becoming untenable so at some point someone just said "I'll keep them local to this function as to avoid having to deal with all that".
I suspect the ultimate solution would be one or more of:
1. Split this file up into multiple more reasonably sized ones
2. Rename things such that it is easier to avoid collisions, or keep namespace names short and fully qualify them
--
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] avantgardnerio merged pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
avantgardnerio merged PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958
--
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] gruuya commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
gruuya commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1081860086
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
Oh, interesting; will check it out 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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] gruuya commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
gruuya commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1081432997
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
That's a fair point; let me see if I can make a self-contained example for the usecase we have and then try my luck with a PR.
Namely, we use `sql_to_expr` to [convert](https://github.com/splitgraph/seafowl/blob/main/src/context.rs#L1173) the `WHERE` clauses in `UPDATE` and `DELETE` statements from sqlparser `Expr`s into Datafusion `Expr`s during logical planning. Subsequently, during physical planning of our custom logical extension nodes for `UPDATE` and `DELETE` we employ these expressions in order to achieve the desired action using available Datafusion primitives (pruning, scanning, filtering, etc.).
--
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 #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1081534824
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
In parallel you may be interested in some work @avantgardnerio is working on here https://github.com/apache/arrow-datafusion/pull/4902 and likely elsewhere to add better INSERT/UPDATE/DELETE support directly to DataFusion
--
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 #4958: Split function
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1072939821
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1636,22 +1636,34 @@ 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;
-
+ /// Convert a sql string into an ast Statement
+ pub fn sql_to_statement(
+ &self,
+ sql: &str,
+ ) -> Result<datafusion_sql::parser::Statement> {
let mut statements = DFParser::parse_sql(sql)?;
- if statements.len() != 1 {
+ 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 statement = statements.pop_front().ok_or_else(|| {
+ DataFusionError::NotImplemented(
+ "The context requires a statement!".to_string(),
+ )
+ })?;
+ Ok(statement)
+ }
+
+ /// Convert an ast Statement into a LogicalPlan
+ pub async fn statement_to_plan(
+ &self,
+ statement: datafusion_sql::parser::Statement,
+ ) -> 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;
Review Comment:
These `use` statements seem out of place 🤔
--
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] ursabot commented on pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#issuecomment-1387356471
Benchmark runs are scheduled for baseline = 7062c2efcf43447545bd5d752ae0692a0e160a31 and contender = 2fdc7b836741fa62f89e9828da65cdda98814fb1. 2fdc7b836741fa62f89e9828da65cdda98814fb1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8582671fc6a94f6986d35e917a57a5e3...64a3e1dfdcb148f798bc2f16ddc6a81d/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/0c5c1b3fcb8e421baa7ac3a40a85b94d...fb5bad39fa754beba354c47d1c58af82/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1ae013c57e714441b5ea0eb3f07e59bb...9a6244f36f0e4d5892359fad5d1f54fb/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/6d030daec00d4f9a99e2bf8434d58af1...35629ee127ac4f998c7180d6ad307b93/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] avantgardnerio commented on a diff in pull request #4958: Expose `sql_to_statement` and `statement_to_plan` on `SessionState`
Posted by GitBox <gi...@apache.org>.
avantgardnerio commented on code in PR #4958:
URL: https://github.com/apache/arrow-datafusion/pull/4958#discussion_r1073760608
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1729,6 +1741,15 @@ impl SessionState {
query.statement_to_plan(statement)
}
+ /// 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> {
+ let statement = self.sql_to_statement(sql)?;
+ let plan = self.statement_to_plan(statement).await?;
+ Ok(plan)
+ }
Review Comment:
@gruuya your suggestion makes sense and I'd be amenable to merging a PR containing the changes you described, but in the interest of expedience, given the approvals on this PR, I'd like to merge it as-is.
If you file an issue, I can take a look next time I'm free.
--
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