omit some clone when converting sql to logical plan #1945

doki23 · 2022-03-08T03:27:09Z

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 :)

xudong963

LGTM, thanks @doki23

xudong963 · 2022-03-08T05:33:45Z

datafusion/src/sql/planner.rs

-            location: location.clone(),
-            file_type: *file_type,
-            has_header: *has_header,
+            name,


👍，clone disappeared

jimexist · 2022-03-08T15:37:42Z

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))


i wonder what's the motivation behind .remove?

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

jimexist · 2022-03-08T15:38:47Z

if we are using so many remove(0) i wonder if we shall switch to VecDeque

jimexist

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.

alamb

I think it looks great -- thank you @doki23

After this PR there is definitely some more cleanup possible such as

    fn sql_expr_to_logical_expr(&self, sql: &SQLExpr, schema: &DFSchema) -> Result<Expr> {

But I can file a follow on ticket for that.

alamb · 2022-03-08T18:50:48Z

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))


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

alamb · 2022-03-08T18:55:19Z

datafusion/src/sql/planner.rs

-        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))


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

doki23 · 2022-03-09T02:07:48Z

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

doki23 · 2022-03-09T02:09:40Z

I accidentally closed this pr, hope it didn't cause confusion😂

doki23 · 2022-03-09T02:34:34Z

I applied the suggestions, please review again @jimexist @alamb

houqp · 2022-03-09T03:47:57Z

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())


how about combining the length check and pop logic into a single block to make it more readable? basically something like below:

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)

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.

datafusion/src/sql/planner.rs

Co-authored-by: xudong.w <wxd963996380@gmail.com>

alamb · 2022-03-09T21:44:00Z

@doki23 and @xudong963 I merged this PR with master to resolve (another) conflict in a0f7091 -- I hope you don't mind. I don't want it to be hanging out too long and picking up conflits along the way

alamb · 2022-03-10T14:01:36Z

Thanks again @doki23

emit some clone

42f54e8

github-actions bot added datafusion Changes in the datafusion crate sql SQL Planner labels Mar 8, 2022

optimize code with clippy

724355a

xudong963 approved these changes Mar 8, 2022

View reviewed changes

houqp approved these changes Mar 8, 2022

View reviewed changes

houqp added the api change Changes the API exposed to users of the crate label Mar 8, 2022

jimexist reviewed Mar 8, 2022

View reviewed changes

jimexist approved these changes Mar 8, 2022

View reviewed changes

alamb approved these changes Mar 8, 2022

View reviewed changes

alamb changed the title ~~emit some clone when converting sql to logical plan~~ omit some clone when converting sql to logical plan Mar 8, 2022

doki23 closed this Mar 9, 2022

doki23 reopened this Mar 9, 2022

make code clearer

e90cf0a

houqp reviewed Mar 9, 2022

View reviewed changes

xudong963 reviewed Mar 9, 2022

View reviewed changes

datafusion/src/sql/planner.rs Outdated Show resolved Hide resolved

doki23 and others added 2 commits March 9, 2022 22:28

resolve conflicts with #1951

b392bc2

Co-authored-by: xudong.w <wxd963996380@gmail.com>

Merge remote-tracking branch 'apache/master' into doki_dev

a0f7091

jimexist approved these changes Mar 10, 2022

View reviewed changes

alamb merged commit 67b8272 into apache:master Mar 10, 2022

alamb mentioned this pull request Mar 10, 2022

Remove some more unecessary cloning in sql_expr_to_logical_expr #1981

Merged

doki23 mentioned this pull request Mar 11, 2022

[follow up]cleaning up statements.remove(0) #1986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

omit some clone when converting sql to logical plan #1945

omit some clone when converting sql to logical plan #1945

doki23 commented Mar 8, 2022

xudong963 left a comment

xudong963 Mar 8, 2022

jimexist Mar 8, 2022

alamb Mar 8, 2022

jimexist commented Mar 8, 2022

jimexist left a comment

alamb left a comment

alamb Mar 8, 2022

alamb Mar 8, 2022

doki23 commented Mar 9, 2022

doki23 commented Mar 9, 2022

doki23 commented Mar 9, 2022

houqp Mar 9, 2022 •

edited

Loading

doki23 Mar 9, 2022 •

edited

Loading

alamb commented Mar 9, 2022

alamb commented Mar 10, 2022

omit some clone when converting sql to logical plan #1945

omit some clone when converting sql to logical plan #1945

Conversation

doki23 commented Mar 8, 2022

Which issue does this PR close?

What changes are included in this PR?

xudong963 left a comment

Choose a reason for hiding this comment

xudong963 Mar 8, 2022

Choose a reason for hiding this comment

jimexist Mar 8, 2022

Choose a reason for hiding this comment

alamb Mar 8, 2022

Choose a reason for hiding this comment

jimexist commented Mar 8, 2022

jimexist left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 8, 2022

Choose a reason for hiding this comment

alamb Mar 8, 2022

Choose a reason for hiding this comment

doki23 commented Mar 9, 2022

doki23 commented Mar 9, 2022

doki23 commented Mar 9, 2022

houqp Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

doki23 Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

alamb commented Mar 9, 2022

alamb commented Mar 10, 2022

houqp Mar 9, 2022 •

edited

Loading

doki23 Mar 9, 2022 •

edited

Loading