Skip to content

fix 593, reduce cloning by taking ownership in logical planner's from fn - #610

Merged
alamb merged 1 commit into
apache:masterfrom
jimexist:fix-593
Jun 27, 2021
Merged

fix 593, reduce cloning by taking ownership in logical planner's from fn#610
alamb merged 1 commit into
apache:masterfrom
jimexist:fix-593

Conversation

@jimexist

@jimexistjimexist commented Jun 23, 2021

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#593

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@jimexistjimexist changed the title fix 593fix 593, reduce cloning by taking ownership in logical planner's from fnJun 23, 2021
alamb
alamb approved these changes Jun 23, 2021
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I really like this change (there is far too much cloning in the creation of plans (and Exprs generally). It is an API change so I think maybe we should leave it open for a few days to see if anyone else has opinions

@alambalamb added the api change Changes the API exposed to users of the crate label Jun 23, 2021
jorgecarleitao
jorgecarleitao approved these changes Jun 23, 2021
View reviewed changes

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Solid improvement to me 👍 Tjhanks @jimexist !

@houqp

houqp commented Jun 24, 2021

Copy link
Copy Markdown
Member

If we really want to make this backwards compatible, we could use impl Into<LogicalPlan> as the argument type, see #615.

@jimexist

jimexist commented Jun 26, 2021

Copy link
Copy Markdown
MemberAuthor

If we really want to make this backwards compatible, we could use impl Into<LogicalPlan> as the argument type, see #615.

thanks for the suggestion. unless it's a goal, i'd like to keep this simple for the moment.

@alamb
alamb merged commit ffb195c into apache:masterJun 27, 2021
@alamb

alamb commented Jun 27, 2021

Copy link
Copy Markdown
Contributor

Thanks again @jimexist

@jimexist
jimexist deleted the fix-593 branch June 27, 2021 12:28
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@viirya
chore: Use proper constructor of IndexShuffleBlockResolver (apache#610)
42ed636
HairstonE pushed a commit to HairstonE/datafusion that referenced this pull request Oct 7, 2025
@mesejo
feat: add execute_stream and execute_stream_partitioned (apache#610)
5e97701
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DandandanDandandanDandandan approved these changes
@alambalambalamb approved these changes
@houqphouqphouqp approved these changes
+1 more reviewer
@jorgecarleitaojorgecarleitaojorgecarleitao approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api changeChanges the API exposed to users of the crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

logical plan builder from method should take owned value as input

5 participants

@jimexist@houqp@alamb@Dandandan@jorgecarleitao