Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

omit some clone when converting sql to logical plan #1945

Merged
merged 5 commits into from
Mar 10, 2022
Merged

omit some clone when converting sql to logical plan #1945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42f54e8
emit some clone
doki23 Mar 8, 2022
724355a
optimize code with clippy
doki23 Mar 8, 2022
e90cf0a
make code clearer
doki23 Mar 9, 2022
b392bc2
resolve conflicts with #1951
doki23 Mar 9, 2022
a0f7091
Merge remote-tracking branch 'apache/master' into doki_dev
alamb Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions datafusion/src/execution/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ impl ExecutionContext {
///
/// This function is intended for internal use and should not be called directly.
pub fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
let statements = DFParser::parse_sql(sql)?;
let mut statements = DFParser::parse_sql(sql)?;

if statements.len() != 1 {
return Err(DataFusionError::NotImplemented(
Expand All @@ -308,7 +308,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())
Copy link
Member

@houqp houqp Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@doki23 doki23 Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Registers a variable provider within this context.
Expand Down
Loading