fix(SubqueryAlias): use maybe_project_redundant_column

[notfilippo]

Which issue does this PR close?

• Closes change_redundant_column lossy approach breaks logical optimizer and physical planner #17405.

Rationale for this change

When creating nested SubqueryAlias operations in complex joins, DataFusion was incorrectly handling column name conflicts by appending suffixes like :1 to duplicate column names. This caused the physical planner to fail with "Input field name {} does not match with the projection expression {}" errors, as the optimizer couldn't properly match columns with these modified names.

The root cause was that the SubqueryAlias creation process was stripping qualification information and mixing columns from left and right sides of joins, leading to name collisions that were resolved by adding numeric suffixes. This approach lost important context needed for proper column resolution.

What changes are included in this PR?

• Replaced the hacky column renaming approach in SubqueryAlias with a projection-based solution
• Added maybe_project_redundant_column function that creates explicit projections with aliases when needed, instead of modifying column names directly
• Removed the maybe_fix_physical_column_name function from the physical planner that was attempting to fix these naming issues downstream
• Updated SubqueryAlias::try_new to use the new projection approach, preserving qualification information properly
• Added test case demonstrating the fix for nested subquery alias scenarios

Are these changes tested?

The changes include a new test case subquery_alias_confusing_the_optimizer that reproduces the original issue and verifies the fix works correctly. Note: The newly added function maybe_project_redundant_column is missing comprehensive tests.

Are there any user-facing changes?

No user-facing changes. This is an internal fix that resolves query planning errors for complex nested join scenarios without changing the public API or query behavior.

[notfilippo]

Seems like the CI flaked. Retrying...

[notfilippo]

cc @LiaCastaneda @berkaysynnada for the review. Thanks!

[LiaCastaneda]

This makes sense to me and is indeed cleaner than the fix from before! just left a few questions I had

[LiaCastaneda]

Here the join.schema() will have the :<N> still no? since we are returning the schema from the top level projection ( if it was inserted ). I know this solves the issue + makes the optimizer pass work but I don't understand how the optimizer works with this fix if we are still keeping the :<N> in the top level schema..

[LiaCastaneda]

I think this solves my doubt above, this is not requalifying the column name but its adding the :<N> as an alias, so I guess the optimizer sees the column names for resultion not the alias?

[LiaCastaneda]

very nit: maybe we can keep a flag inside the iteration above? so we avoid this one

[LiaCastaneda]

Also, I was looking into the logical optimizer and there is this rule to remove unnecessary projections, I don't think it will remove this one but maybe worth checking?

[notfilippo]

I don't think this is an unnecessary projection as the aliases are needed for the correct semantic representation of left.count(Int64(1)):1 in the final projection which actually refers to right.count(Int64(1)) which gets aliased to left.

[alamb]

FWIW you can test stuff like this by writing .slt tests that have EXPLAIN in them and viewing the final output plan

[LiaCastaneda]

This LGTM, When you are available, would you take a look @berkaysynnada ? 🙇‍♀️

[alamb]

@NGA-TRAN could you help review this PR?

[notfilippo]

Rebased to latest main

[NGA-TRAN]

I am actively reviewing this

[NGA-TRAN]

Very nice work, @notfilippo
I only have some suggestions to add more comments to explain the bugs and the fix

[NGA-TRAN]

a, a, b, b, a, a:1 becomes _, a:1, _, b:1, a:2, a:1:1? Does it mean we keep the last of the same contiguous values and add 1, 2, 3, ... into the same but non contiguous?
Can you add this comment on top of the test?

[NGA-TRAN]

Can you add example in this comment? Something like you use in the test below:
a, a, b, b, a, a:1 becomes _, a:1, _, b:1, a:2, a:1:1

It is also good to explain why you do this. Maybe another more general example for it?

[NGA-TRAN]

For other reviewers, this is a reproducer of the bug. I have run this test before the fix and it fails. Great job to reproduce this with logical and physical plan @notfilippo.

I suggest you add the below comment so we know what the bug is

[NGA-TRAN]

Suggested change

	async fn subquery_alias_confusing_the_optimizer() -> Result<()> {
	// Reproducer for DataFusion issue #17405:
	//
	// The following SQL is semantically invalid. Notably, the `SELECT left_table.a, right_table.a` clause is missing from the explicit logical plan:
	//
	// SELECT a FROM (
	// -- SELECT left_table.a, right_table.a
	// FROM left_table
	// FULL JOIN right_table ON left_table.a = right_table.a
	// ) AS alias
	// GROUP BY a;
	//
	// As a result, the variables within `alias` subquery are not properly distinguished, which leads to a bug when converting the logical plan into a physical plan.
	//
	// The proposed fix is to implicitly insert a ProjectionExec node to represent the missing SELECT clause.
	async fn subquery_alias_confusing_the_optimizer() -> Result<()> {

[notfilippo]

Thanks for the insight @NGA-TRAN, I've added the comments and the code looks much better when contextualised.

[NGA-TRAN]

Looks awesome. Thanks @notfilippo

[NGA-TRAN]

This existing test also help verify the fix

[alamb]

Thanks for the code @notfilippo and for the reviews @LiaCastaneda and @NGA-TRAN 🙏

[alamb]

FWIW you can test stuff like this by writing .slt tests that have EXPLAIN in them and viewing the final output plan

[hareshkh]

FYI - cherry-picking this onto branch-50 for 50.3.0 here - #18130 @notfilippo