Convert SqlToRel::statement_to_pan to take ownership rather than references.

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
While working on some other code, I noticed that SqlToRel::statement_to_plan takes a reference to the parse tree -- this results in having to clone() many values (identifiers, etc) while converting a parsed SQL query to a LogicalPlan

The only callsite I could find throws away the parsed query immediately; Doing this would likely save lots of cloning internal to SqlToRel

Describe the solution you'd like

If someone wanted a fun project, they could convert SqlToRel::statement_to_pan to take ownership rather than references.
https://github.com/apache/arrow-datafusion/blob/0052667afae33ba9e549256d0d5d47e2f45e6ffb/datafusion/src/sql/planner.rs#L121

Specifically, change:

    pub fn statement_to_plan(&self, statement: &DFStatement) -> Result<LogicalPlan> {

To

    pub fn statement_to_plan(&self, statement: DFStatement) -> Result<LogicalPlan> {

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
This would be a software engineering exercise -- it wouldn't really require much in the way of knowledge of the guts of DataFusion

@alamb

@alamb good catch!

Here I give a specific example to omit clone

    pub fn external_table_to_plan(
        &self,
        statement: &CreateExternalTable,
    ) -> Result<LogicalPlan> {
        let CreateExternalTable {
            name,
            columns,
            file_type,
            has_header,
            location,
        } = statement;

        ...

        Ok(LogicalPlan::CreateExternalTable(PlanCreateExternalTable {
            schema: schema.to_dfschema_ref()?,
            name: name.clone(),
            location: location.clone(),
            file_type: *file_type,
            has_header: *has_header,
        }))
    }

to

    pub fn external_table_to_plan(
        &self,
        statement: CreateExternalTable,
    ) -> Result<LogicalPlan> {
        let CreateExternalTable {
            name,
            columns,
            file_type,
            has_header,
            location,
        } = statement;

       ...

        Ok(LogicalPlan::CreateExternalTable(PlanCreateExternalTable {
            schema: schema.to_dfschema_ref()?,
            name,
            location,
            file_type: *file_type,
            has_header: *has_header,
        }))
    }

BTW, I think it's better to add a good first issue

@alamb

I wanna take this issue, can I? @alamb

@doki23

Please do @doki23 !

docs: Improve documentation for running stability plan tests (apache#…

deprecate: mark Statistics V2 framework (PR apache#14699) as deprecat…

Navigation Menu

Repository navigation

Convert `SqlToRel::statement_to_pan` to take ownership rather than references. #1469

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Convert SqlToRel::statement_to_pan to take ownership rather than references. #1469

Description

Activity

xudong963 commented on Dec 21, 2021

doki23 commented on Feb 28, 2022

alamb commented on Feb 28, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Convert `SqlToRel::statement_to_pan` to take ownership rather than references. #1469