Minor: Avoid cloning as many `Ident` during SQL planning #4534

alamb · 2022-12-06T21:35:00Z

~~Draft as it builds on #4530~~

Which issue does this PR close?

N/A

Rationale for this change

I noticed a bunch of redundant copying while working on #4530 but wanted to keep that PR smaller

What changes are included in this PR?

Remove redundant cloning

Are these changes tested?

covered by existing tests

Are there any user-facing changes?

alamb · 2022-12-06T21:35:49Z

datafusion/sql/src/utils.rs

-// Normalize an identifier to a lowercase string unless the identifier is quoted.
-pub(crate) fn normalize_ident(id: &Ident) -> String {
-    match id.quote_style {
-        Some(_) => id.value.clone(),


here the value is always cloned which is not necessary for most uses when we already have an owned string

alamb · 2022-12-07T19:14:12Z

datafusion/sql/src/planner.rs

            for cte in with.cte_tables {
                // A `WITH` block can't use the same name more than once
-                let cte_name = normalize_ident(&cte.alias.name);
+                let cte_name = normalize_ident(cte.alias.name.clone());


previously normalize_indent always cloned. Now it only clones in a few places and most of the time can reuse the String in the sqlparser-ast directly

alamb · 2022-12-07T19:28:08Z

datafusion/sql/src/planner.rs

                .any(|x| x.option == ColumnOption::Null);
            fields.push(Field::new(
-                &normalize_ident(&column.name),
+                &normalize_ident(column.name),


This is unfortunate to simply drop the String immediately, but Field::new requires a &str (it can't take the String). Filed upstream: https://github.com/apache/arrow-rs/pull/3288/files

Agree, Look like we can do improvement like above SubqueryAlias::try_new().

…ized_indent

alamb

@jackwener I wonder if you have time or interest to review this PR?

alamb · 2022-12-07T19:28:51Z

datafusion/sql/src/planner.rs

        }
    }

-    fn apply_expr_alias(plan: LogicalPlan, idents: &Vec<Ident>) -> Result<LogicalPlan> {


This function took a reference but the caller immediately drops the actual Vec. This PR reuses it

alamb · 2022-12-11T11:30:57Z

datafusion/expr/src/logical_plan/plan.rs

-    pub fn try_new(plan: LogicalPlan, alias: &str) -> datafusion_common::Result<Self> {
+    pub fn try_new(
+        plan: LogicalPlan,
+        alias: impl Into<String>,


This change allows SubqueryAlias::try_new to take a String if the caller has one or a &str that will be copied into a new String if needed

Nice implementation👍, copy just when call &str.

jackwener

I review them carefully, It make sense to me.
Thanks @alamb.

jackwener · 2022-12-11T13:49:34Z

datafusion/expr/src/logical_plan/plan.rs

-    pub fn try_new(plan: LogicalPlan, alias: &str) -> datafusion_common::Result<Self> {
+    pub fn try_new(
+        plan: LogicalPlan,
+        alias: impl Into<String>,


Nice implementation👍, copy just when call &str.

jackwener · 2022-12-12T03:04:38Z

BTW, I think we can also avoid clone in some method in LogicalPlanBuilder().
We can it in followup PR.

alamb · 2022-12-12T14:40:56Z

Thank you for the review @jackwener

ursabot · 2022-12-13T16:25:01Z

Benchmark runs are scheduled for baseline = 4ecf3e7 and contender = 2457ce4. 2457ce4 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

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Dec 6, 2022

alamb commented Dec 6, 2022

View reviewed changes

alamb mentioned this pull request Dec 6, 2022

Fix querying and defining table / view names with period #4530

Merged

Minor: Avoid cloning some Identifiers during planning

895b4cd

alamb force-pushed the alamb/remove_normalized_indent branch from d4f1c9c to 895b4cd Compare December 7, 2022 19:13

github-actions bot removed sqllogictest SQL Logic Tests (.slt) logical-expr Logical plan and expressions core Core DataFusion crate labels Dec 7, 2022

alamb commented Dec 7, 2022

View reviewed changes

alamb mentioned this pull request Dec 7, 2022

Minor: Allow Field::new and Field::new_with_dict to take existing String as well as &str apache/arrow-rs#3288

Merged

alamb commented Dec 7, 2022

View reviewed changes

alamb mentioned this pull request Dec 7, 2022

minor: Remove more clones from the planner #4546

Merged

alamb changed the title ~~Minor: Avoid cloning some Ident as much during planning~~ Minor: Avoid cloning as many Ident during SQL planning Dec 7, 2022

alamb marked this pull request as ready for review December 7, 2022 19:36

alamb added 2 commits December 11, 2022 06:27

Merge remote-tracking branch 'apache/master' into alamb/remove_normal…

fbd2cab

…ized_indent

Remove another copy

0c0a2fc

github-actions bot added the logical-expr Logical plan and expressions label Dec 11, 2022

alamb commented Dec 11, 2022

View reviewed changes

jackwener approved these changes Dec 11, 2022

View reviewed changes

alamb merged commit 2457ce4 into apache:master Dec 12, 2022

alamb deleted the alamb/remove_normalized_indent branch December 12, 2022 21:15

Minor: Avoid cloning as many Ident during SQL planning #4534

Minor: Avoid cloning as many Ident during SQL planning #4534

Uh oh!

Conversation

alamb commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackwener Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

alamb Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

jackwener Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

jackwener Dec 11, 2022

Choose a reason for hiding this comment

Uh oh!

jackwener commented Dec 12, 2022

Uh oh!

alamb commented Dec 12, 2022

Uh oh!

ursabot commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Minor: Avoid cloning as many `Ident` during SQL planning #4534

Minor: Avoid cloning as many `Ident` during SQL planning #4534

alamb commented Dec 6, 2022 •

edited

Loading

alamb Dec 7, 2022 •

edited

Loading