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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 27, 2021
Merged

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

merged 1 commit into from
Jun 27, 2021

Conversation

jimexist
Copy link
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?

@jimexist jimexist changed the title fix 593 fix 593, reduce cloning by taking ownership in logical planner's from fn Jun 23, 2021
@alamb alamb added the datafusion Changes in the datafusion crate label Jun 23, 2021
alamb
alamb approved these changes Jun 23, 2021
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

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

@alamb alamb 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
Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

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
Copy link
Member

houqp commented Jun 24, 2021

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

@jimexist
Copy link
Member Author

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:master Jun 27, 2021
@alamb
Copy link
Contributor

alamb commented Jun 27, 2021

Thanks again @jimexist

@jimexist jimexist deleted the fix-593 branch June 27, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgecarleitao jorgecarleitao jorgecarleitao approved these changes

@houqp houqp houqp approved these changes

@alamb alamb alamb approved these changes

@Dandandan Dandandan Dandandan approved these changes

Assignees
No one assigned
Labels
api change Changes the API exposed to users of the crate datafusion Changes in the datafusion 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