Reuse last projection layer when renaming columns #14684

blaginin · 2025-02-15T18:43:55Z

Which issue does this PR close?

Related to #14563

Rationale for this change

Currently, when with_column_renamed is called, a new Projection layer is always created. For example, in the dataframe benchmark, logical plan looks like this:

These layers do not affect the query itself (as they are removed by optimize_projections), but they make renaming new columns (and optimization itself) quite slow.

What changes are included in this PR?

I added an optimization for one edge case - when there's already a projection on top and we're adding a new one. This is a common scenario, especially when renaming many columns (as in the dataframe benchmark). Instead of adding a new projection layer on top, we replace the existing one if possible.

Are these changes tested?

Extended test case

Are there any user-facing changes?

No

blaginin · 2025-02-15T18:44:17Z

group                                                main                                    one-layer
with_column_10                                       1.39   764.9±10.37µs        ? ?/sec     1.00    550.0±4.71µs        ? ?/sec
with_column_100                                      3.88  1490.1±22.27ms        ? ?/sec     1.00    384.5±3.24ms        ? ?/sec
with_column_200                                      5.63      36.6±0.43s        ? ?/sec     1.00       6.5±0.63s        ? ?/sec

Omega359 · 2025-02-16T22:18:24Z

Quite a speed improvement! I'll look at the code in a bit

Omega359 · 2025-02-16T22:21:43Z

datafusion/core/tests/dataframe/mod.rs

+        .with_column_renamed("c4", "boom")?;
+
+    assert_eq!("\
+        Projection: aggregate_test_100.c1 AS one, aggregate_test_100.c2 AS two, aggregate_test_100.c3, aggregate_test_100.c2 + aggregate_test_100.c3 AS sum AS total\


AS sum AS total is valid?

Thank you for the thorough reading 🙏 AS sum AS total happens because there's an alias over an alias - I believe it's harmless since it just wraps the original expression.

There's a function unalias_nested that can resolve this, but I don't want to call it because it's recursive and expensive. A potentially better option IMO is to reuse the existing alias in alias_qualified and alias if it's already there (similar to this PR). This feels like a separate improvement from this PR though

I just worry that another column referenced 'sum' and then it is aliased to 'total' and what the behaviour of that would be.

But I don't think that's possible? "total" cannot be referenced within the same projection, because for that projection it is not in its input. But to display it in a nicer way, I'll do #14781

Omega359 · 2025-02-16T22:25:13Z

datafusion/core/src/dataframe/mod.rs

+                        SchemaError::FieldNotFound { .. },
+                        _,
+                    )) => {
+                        self.plan = LogicalPlan::Projection(


If the field is not found why are we not just returning Ok(self)? I am unsure why we would need to add a projection for the plan in this case

We need to do that because self was destructed and so I need to put plan back. try_new_with_schema is quite cheap and this shouldn't be happening often anyway

Omega359 · 2025-02-16T22:29:28Z

datafusion/core/src/dataframe/mod.rs

+                })
+                .collect();
+            LogicalPlan::Projection(Projection::try_new(expr, input)?)
+        } else {


There seems to be a lot of duplication here. I wonder if there is a way we could reduce that? 🤔

That's very fair! I also dislike repeating qualifier_rename, field_rename twice, but it's happening because they reference plan and must come after its destruction (hence needed twice)

alamb

What is the status of this PR?

I took a brief look at this PR and it seems like it may have been superceded by this one from @Omega359

Dataframe with_column and with_column_renamed performance improvements #14653

blaginin · 2025-03-05T12:02:55Z

Hey, I still want to merge this one. #14653 has indeed made the benches much faster, but I think it's still good to make the logical plan smaller if we can

alamb · 2025-03-05T20:54:02Z

Looks to me like there are a few outstanding items:

Reduce the code repetition if possible
Merge up to resolve conflicts

blaginin · 2025-03-17T17:55:58Z

Want to merge #14781 and #15159 first so making this as a draft

github-actions · 2025-05-18T02:13:21Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

Omega359 · 2025-05-23T16:28:22Z

@blaginin I see that #14781 is still draft, this PR stillok otherwise?

blaginin · 2025-05-24T12:11:08Z

@blaginin I see that #14781 is still draft, this PR stillok otherwise?

yes, thanks for the reminder - there was some work happening around alias changes with unnest, so I left it stale. But I think this PR can go out now to make the logical plans nicer.

alamb

Thank you for this contribution @blaginin

alamb · 2025-05-27T20:04:23Z

datafusion/core/src/dataframe/mod.rs

-        let project_plan = LogicalPlanBuilder::from(self.plan)
-            .project_with_validation(projection)?
-            .build()?;
+        let project_plan = if let LogicalPlan::Projection(Projection {


to be clear, the projections are flattened as part of the planning process right? So this PR mostly is about making logical EXPLAIN plans look nicer with with_column_renamed?

If so it seems like a lot of complexity for (just) that feature. Maybe we can switch to showing optimized explain plans or something for people who want to see a simpler version

I think it may have been related to trying to improve planning performance by reducing the number of projections up front so planning doesn't need to do it. Improving the logical plan output is definitely nice though :)

Reuse last projection layer when renaming column

800c9ac

github-actions bot added the core Core DataFusion crate label Feb 15, 2025

Omega359 reviewed Feb 16, 2025

View reviewed changes

merge main

159afad

blaginin marked this pull request as ready for review February 17, 2025 19:08

blaginin requested a review from Omega359 February 19, 2025 14:59

blaginin mentioned this pull request Feb 19, 2025

Reuse alias if possible #14781

Draft

alamb mentioned this pull request Mar 4, 2025

Weekly Plan (Andrew Lamb) March 3, 2025 #14978

Closed

12 tasks

alamb reviewed Mar 5, 2025

View reviewed changes

blaginin marked this pull request as draft March 17, 2025 17:56

github-actions bot added the Stale PR has not had any activity for some time label May 18, 2025

github-actions bot added sqllogictest SQL Logic Tests (.slt) proto Related to proto crate labels May 23, 2025

blaginin added 2 commits May 23, 2025 17:53

Merge main

b9d6ad9

Format file

2fd800c

blaginin force-pushed the reuse-projection branch from d56e5f9 to 2fd800c Compare May 23, 2025 16:53

github-actions bot removed sqllogictest SQL Logic Tests (.slt) proto Related to proto crate labels May 23, 2025

blaginin self-assigned this May 23, 2025

Update the test macros

0dee897

blaginin removed the Stale PR has not had any activity for some time label May 23, 2025

blaginin marked this pull request as ready for review May 24, 2025 12:08

Merge branch 'main' into reuse-projection

d1c6db9

alamb reviewed May 27, 2025

View reviewed changes

Reuse last projection layer when renaming columns #14684

Are you sure you want to change the base?

Reuse last projection layer when renaming columns #14684

Conversation

blaginin commented Feb 15, 2025

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!

blaginin commented Feb 15, 2025

Uh oh!

Omega359 commented Feb 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Omega359 Feb 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

blaginin commented Mar 5, 2025

Uh oh!

alamb commented Mar 5, 2025

Uh oh!

blaginin commented Mar 17, 2025

Uh oh!

github-actions bot commented May 18, 2025

Uh oh!

Omega359 commented May 23, 2025

Uh oh!

blaginin commented May 24, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Omega359 Feb 16, 2025 •

edited

Loading