Reuse last projection layer when renaming columns

[blaginin]

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]

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]

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

[Omega359]

AS sum AS total is valid?

[blaginin]

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

[Omega359]

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

[blaginin]

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]

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

[blaginin]

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]

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

[blaginin]

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]

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]

Looks to me like there are a few outstanding items:

1. Reduce the code repetition if possible
2. Merge up to resolve conflicts