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/03/08 03:27:24 UTC
[GitHub] [arrow-datafusion] doki23 opened a new pull request #1945: emit some clone when converting sql to logical plan
doki23 opened a new pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945
# Which issue does this PR close?
Closes #1469.
# What changes are included in this PR?
It's an api change.
Take ownership of sql-parser components to emit some clones.
It's hard to me - a new comer of rust and df, and the mutual calls between functions are very complex, so please bear with me :)
--
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 change in pull request #1945: emit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r821972537
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -2053,8 +2058,8 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
select_list, where_clause
);
- let rewrite = DFParser::parse_sql(&query)?;
- self.statement_to_plan(&rewrite[0])
+ let mut rewrite = DFParser::parse_sql(&query)?;
+ self.statement_to_plan(rewrite.remove(0))
Review comment:
I realize this was not added in this PR, but I wonder if we could add an `assert_eq!(rewrite.len(), 1)` here to make sure we don't accidentally lose one of these statements later on
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.remove(0))
Review comment:
I think it is to take ownership of the 1 item from `statements`. `statements.len() == 1` is verified on line 305 above
I think `statements.pop().unwrap()` is equivalent and might be clearer what is going on, though I think `remove(0)` is also fine
--
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] doki23 commented on a change in pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r822282113
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.pop().unwrap())
Review comment:
Hm, the error description means length of statements should eq 1, but 'statements.pop().ok_or(...)' only indicates that it's >= 1. And I have not found api having same semantics.
--
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] doki23 commented on pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1062481334
> i can approve to unblock merging but i do think the `remove(0)` makes the logic quite cryptic. might need a follow up PR to clean up.
+1
--
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] doki23 commented on a change in pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r822282113
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.pop().unwrap())
Review comment:
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] alamb merged pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945
--
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] xudong963 commented on a change in pull request #1945: emit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
xudong963 commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r821335301
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -331,10 +331,10 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
Ok(LogicalPlan::CreateExternalTable(PlanCreateExternalTable {
schema: schema.to_dfschema_ref()?,
- name: name.clone(),
- location: location.clone(),
- file_type: *file_type,
- has_header: *has_header,
+ name,
Review comment:
👍，`clone` disappeared
--
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] Jimexist commented on pull request #1945: emit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
Jimexist commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1061912593
if we are using so many `remove(0)` i wonder if we shall switch to [`VecDeque`][1]
[1]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html
--
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] houqp commented on a change in pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r822263562
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.pop().unwrap())
Review comment:
how about combining the length check and pop logic into a single block to make it more readable? basically something like below:
```rust
let stmt = statements.pop().ok_or(Err(DataFusionError::NotImplemented(
"The context currently only supports a single SQL statement".to_string()
)))?;
```
--
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 #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1064095311
Thanks again @doki23
--
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] doki23 commented on pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1062495323
I applied the suggestions, please review again @Jimexist @alamb
--
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] doki23 commented on pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1062482290
I accidentally closed this pr, hope it didn't cause confusion😂
--
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] houqp commented on a change in pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r822263562
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.pop().unwrap())
Review comment:
how about combining the length check and pop logic into a single block to make it more readable? basically something like below:
```rust
let stmt = statements.pop().ok_or(Err(DataFusionError::NotImplemented(
"The context currently only supports a single SQL statement".to_string()
)))?;
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
query_planner.statement_to_plan(stmt)
```
--
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] xudong963 commented on a change in pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
xudong963 commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r822345217
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -194,7 +194,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
{
Ok(LogicalPlan::DropTable(DropTable {
name: names.get(0).unwrap().to_string(),
- if_exist: *if_exists,
+ if_exist: if_exists,
Review comment:
after you solve conflicts with https://github.com/apache/arrow-datafusion/pull/1951#discussion_r821440524
```suggestion
if_exists,
```
--
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] Jimexist commented on a change in pull request #1945: emit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#discussion_r821788856
##########
File path: datafusion/src/execution/context.rs
##########
@@ -311,7 +311,7 @@ impl ExecutionContext {
// create a query planner
let state = self.state.lock().clone();
let query_planner = SqlToRel::new(&state);
- query_planner.statement_to_plan(&statements[0])
+ query_planner.statement_to_plan(statements.remove(0))
Review comment:
i wonder what's the motivation behind `.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
[GitHub] [arrow-datafusion] doki23 closed pull request #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
doki23 closed pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945
--
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 #1945: omit some clone when converting sql to logical plan
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1945:
URL: https://github.com/apache/arrow-datafusion/pull/1945#issuecomment-1063402086
@doki23 and @xudong963 I merged this PR with master to resolve (another) conflict in https://github.com/apache/arrow-datafusion/pull/1945/commits/a0f709128c125e50c42dc21cf031f964f4a56f96 -- I hope you don't mind. I don't want it to be hanging out too long and picking up conflits along the way
--
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