fix: Remove dangling table references in unparser

[peasee]

Which issue does this PR close?

Closes #13027

Rationale for this change

This change ensures that table references which don't exist at their current level are removed. For example, SELECT ta.j1_id FROM (SELECT j1_id FROM j1 ta) is invalid because the subquery is un-aliased so ta is not a valid reference at the top-level projection.

This is usually caused by derived subqueries, both un-aliased and aliased.

What changes are included in this PR?

• For each select_to_sql_expr, collects the available table identifiers at that level from the projection and table joins.
• Iterates over the projection and order by and compares them to the list of collected identifiers. For identifiers that are not found, strips their table reference leaving just the bare column (e.g. ta.j1_id -> j1_id).

Are these changes tested?

Yes. A collection of new plan-to-SQL roundtrip tests to validate the changes.

Are there any user-facing changes?

No

[sgrebnov]

🚀

[alamb]

Thanks for this contribution @peasee and for the review @sgrebnov

I may not understand something but I left some suggestions for your consideration

[alamb]

You can probably avoid a bunch of copies if you made this return a reference to a &str rather than a String -- if the caller needed the string they can always copy it.

Suggested change

	pub fn get_alias(&self) -> Option<String> {
	pub fn get_alias(&self) -> Option<&str> {

[peasee]

It looks like Ident doesn't implement anything that would return a &str, so it needs a String intermediary. I'm also not sure what copies you're referring too, I don't make any copies of the values from collect_valid_idents? The return from get_alias also isn't cloned, and is taken ownership of by collect_valid_idents.

[alamb]

I don't understand why this needs to have a clone now 🤔

[peasee]

Because twj.relation() takes ownership of the relation_builder causing it to move, so we can't borrow it again later.

The only thing I use the relation builder for is to retrieve the list of all the identifiers, so I could probably do that before the twj.relation() then just pass those like:

let valid_idents = select_builder.collect_valid_idents();
twj.relation();

which shouldn't require a clone.

[alamb]

I don't understand this code super deeply, but this seems to me like it is treating the symptom (incorrect qualifiers) rather than the root cause.

Specifically, did you look into fixing the code so that it didn't create incorrect indentifiers in the first place, rather than trying to modify the created AST after the fact to remove incorrect indentifers ?

[peasee]

Yeah, I had taken a look into doing this at the unparser LogicalPlan level but I wasn't making very good progress. It could be my lack of understanding with LogicalPlan, but I think the symptom originates from the parser rather than the unparser.

If you'd be open to merging this still as an AST modifier, perhaps we could gate it behind a dialect option or feature flag as a non-default?

[alamb]

Maybe @sgrebnov or @phillipleblanc can offer some advice about how to proceed with this PR -- I feel like it adds significantly complexity and I am not sure it doesn't also introduce some hard to understand subtle bugs with indentifiers

[phillipleblanc]

Maybe @sgrebnov or @phillipleblanc can offer some advice about how to proceed with this PR -- I feel like it adds significantly complexity and I am not sure it doesn't also introduce some hard to understand subtle bugs with indentifiers

I plan to take a shot at preventing the identifiers from getting introduced in the first place and/or adding subquery alias nodes as appropriate. I spent a little time on it this weekend and was able to get the bare aggregation + table scan working.

@peasee let's close this PR for now.