fix: avoid invalid qualifiers in unparsed subqueries - #24808
fix: avoid invalid qualifiers in unparsed subqueries#24808blinding-pixels wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24808 +/- ##
==========================================
- Coverage 81.52% 81.52% -0.01%
==========================================
Files 1123 1123
Lines 406148 406193 +45
Branches 406148 406193 +45
==========================================
+ Hits 331124 331159 +35
- Misses 55659 55665 +6
- Partials 19365 19369 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @blinding-pixels for working on this. I have left few comments for your consideration.
| let requires_alias = self.dialect.requires_derived_table_alias(); | ||
| let alias = requires_alias | ||
| .then(|| self.new_table_alias(input_alias.to_string(), vec![])); | ||
| self.derive(p.input.as_ref(), relation, alias, false)?; |
There was a problem hiding this comment.
The new scope rewrite changes only the SELECT items. An outer Sort is rendered before this branch and keeps the old qualifier.
For example, SELECT j1_id FROM (...) ORDER BY j1_id can still emit ORDER BY ta.j1_id. Only derived_projection is visible there.
The same problem applies to an outer filter rendered before this branch. Please rebase all outer clauses and add an ORDER BY regression.
| self.flatten_table_aliases.iter().any(|a| a == alias) | ||
| } | ||
|
|
||
| pub fn enter_subquery_alias(&mut self) { |
There was a problem hiding this comment.
These methods are public because ast is a public module. Only the unparser uses this scope state.
Please use pub(super) for these three methods.
3 participants
Which issue does this PR close?
Rationale for this change
I noticed that this issue had an earlier PR from almost two years ago. That implementation was closed because it used broad identifier cleanup, and maintainers were concerned it could introduce subtle bugs.
Since then, DataFusion has added much of the machinery needed for the approaches recommended in that review. This solution uses that newer machinery and combines both approaches. It removes the qualifier when the derived table does not need an alias, and rewrites the reference to the derived alias when the dialect requires one.
What changes are included in this PR?
This PR detects when a projection enters a new derived-table scope and prevents its outer expressions from referring to an inner table alias that is no longer visible.
For dialects that do not require a derived-table alias, the invalid inner qualifier is removed. For dialects such as MySQL that require an alias, the outer reference is rewritten to the generated derived-table alias. Explicitly named subquery scopes remain unchanged.
What is the testing strategy for this PR?
The SQL unparser round-trip tests cover the issue's original query with both the generic and MySQL dialects. They also cover filtered and distinct derived inputs, and update existing limit and nested-projection cases to assert valid outer references.
The required formatting, Clippy, and extended workspace test suite all pass, including all 505 SQL logic test files.
Are there any user-facing changes?
Yes. SQL produced by the unparser no longer contains qualifiers that refer to tables outside their visible scope. Dialects that require derived-table aliases now qualify the outer reference with the generated alias. There are no public API changes.