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.

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

minor: Remove more clones from the planner #4546

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Dec 7, 2022

Draft as it builds on #4534

Which issue does this PR close?

N/A

Rationale for this change

Less cloning is better

What changes are included in this PR?

Allow subquery_alias and subquery_alias_owned to take an existing String rather than always copying it

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

@alamb alamb marked this pull request as draft December 7, 2022 19:36
@github-actions github-actions bot added logical-expr Logical plan and expressions sql SQL Planner labels Dec 7, 2022
@alamb alamb changed the title minor: Remove more clones from the planner @alamb minor: Remove more clones from the planner Dec 7, 2022
@alamb alamb marked this pull request as ready for review December 8, 2022 18:01
@alamb alamb marked this pull request as draft December 8, 2022 18:01
@alamb alamb force-pushed the alamb/remove_more_clones branch from 685e75c to ee5c4c4 Compare December 12, 2022 14:58
@alamb alamb marked this pull request as ready for review December 12, 2022 16:18
use std::sync::Arc;

/// Create a column expression based on a qualified or unqualified column name
pub fn col(ident: &str) -> Expr {
///
/// example:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is demonstrating you can still call col with a &str but now also with a String or &String to allow backwards compatibility as well as reuse

@@ -70,7 +71,7 @@ impl Column {
// name
Column {
relation: None,
name: String::from(flat_name),
name: flat_name,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would always clone previously, and now it allows the caller to pass in a String if they have one and avoid the clone

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Nice job, thanks @alamb ❤️

@alamb alamb merged commit 1ddcafe into apache:master Dec 13, 2022
@ursabot
Copy link

ursabot commented Dec 13, 2022

Benchmark runs are scheduled for baseline = 0de7d81 and contender = 1ddcafe. 1ddcafe is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@alamb alamb deleted the alamb/remove_more_clones branch August 8, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants