Repository navigation
Conversation
A correlated filter pulled through a SubqueryAlias kept the inner table qualifier. When that alias differed from the table name, the join condition named a column the rewritten subquery no longer had. Fixes apache#25837 Assisted by Cursor (Grok).
github-actions
Bot
added
optimizer
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @canyang25 for working on this
Left a comment please take a look.
| if let Expr::Column(col) = &expr | ||
| && cols.contains(col) | ||
| { | ||
| let new_col = Column::new(Some(alias.clone()), col.name.clone()); |
There was a problem hiding this comment.
Keeping col.name is incorrect when pull-up adds a correlation column whose name matches another projected column. SubqueryAlias::try_new disambiguates duplicate field names using suffixes such as id:1, so changing only the qualifier can target a different column.
Use the rebuilt alias’s actual input-to-output column mapping for both the filters and propagated correlation columns, and add a collision regression case.
Pull-up can add a correlation column whose name matches another projected column. SubqueryAlias suffixes the duplicate (id:1), so requalifying only the qualifier targets the wrong column. Map both the join filters and the propagated correlation columns through the rebuilt alias's positional output names. Assisted by Cursor (Grok).
The lateral join and EXISTS/IN rewrites wrapped the pulled-up subquery with SubqueryAlias::try_new, then requalified filters by keeping the inner column name. When try_new suffixes a duplicate (id, id:1, id:2), that name is an earlier column. Both rewrites now use the same positional input-to-output mapping as alias_output_column, and a correlated column missing from the pre-alias schema is an error. Assisted by Cursor (Grok).
requalify_filter mapped both the pulled-up correlation predicate and the user's ON clause through the pre-alias schema. ON is already bound to the alias output. When an inner column shares that qualifier, the map sends ON s.id to a later id:1. Pull-up only appends columns, and those are the only names unique_field_aliases suffixes, so the planner's ON names stay valid. EXISTS, IN, and scalar decorrelation only positionally rewrite correlation columns, not a user predicate that already uses output names. Assisted by Cursor (Grok).
|
Thanks @kumarUjjawal, good catch. The fix now uses the alias's real input-to-output column mapping, so a renamed duplicate like id:1 is handled correctly. This covers LATERAL, EXISTS/IN, and scalar subqueries. The user's ON clause is left unchanged. I added collision tests to lateral_join.slt and subquery.slt. |
|
@canyang25 |
Assisted by Cursor (Grok).
Thanks @kosiew! I merged main and resolved the conflicts. It was just one small spot in decorrelate.rs, and I kept both sides' fields. Ready for review whenever you have time. |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for addressing the correlated-column resolution issue across subquery aliases. The change fixes the root cause by mapping pulled-up predicates to the actual alias output columns, including duplicate-name suffixes. The new regression tests cover the reported failure and related correctness cases.
The architectural approach is appropriate. Column identity must be preserved at each alias boundary, where qualifiers and output names can change. Tracking filters by alias subtree prevents unrelated predicates from being rewritten, while preserving already-resolved LATERAL ON expressions maintains the distinction between internal correlation predicates and user-authored join conditions. The additional complexity is justified by these ownership and schema invariants.
I approve this change. I found no blocking issues. The inline suggestions concern additional regression coverage and a small behavior-preserving simplification; neither needs to delay merging.
| // column already has. `SubqueryAlias::try_new` suffixes that | ||
| // duplicate (`id` becomes `id:1`), so the output name is the | ||
| // field at the same position, not the inner name. | ||
| let Some(base) = self.alias_filter_base.pop() else { |
There was a problem hiding this comment.
Optional follow-up: could we add result-based LATERAL tests for a correlated filter crossing two nested derived aliases and for two correlated sibling derived tables? These would exercise the alias_filter_base stack and filter-slice isolation. Please use distinct correlation keys and expected results for the siblings so incorrect cross-subtree remapping is observable.
| return Ok(()); | ||
| } | ||
| for filter in filters { | ||
| *filter = filter |
There was a problem hiding this comment.
Optional simplification: could this reuse utils::replace_qualified_name(filter.clone(), cols, input_schema, output_schema) instead of duplicating its expression transformer? Both use cols.contains and alias_output_column. Please retain the slice boundary and empty fast path; the lateral mapper has different eligibility rules and can remain separate.
|
Marking as draft as this has a bunch of conflicts and has some feedback to address Thank you @canyang25 and @kosiew |
| /// Filters appended while that alias is being rewritten belong to its | ||
| /// subtree. On the way up, only those filters are requalified to the | ||
| /// alias, so a sibling filter that names the same column is left alone. | ||
| alias_filter_base: Vec<usize>, |
There was a problem hiding this comment.
I can't help but feel this decorrelate rule is growing too complicated with all these special cases. I feel like rather than patching all these corner cases, we need to start considering reworking how we decorreleate these joins in general, see
Assisted by Cursor (Grok).
Assisted by Cursor (Grok).
Assisted by Cursor (Grok).
Which issue does this PR close?
Rationale for this change
A LATERAL subquery whose correlated filter goes through a derived-table alias failed to plan. When the filter was pulled up through the
SubqueryAlias, it kept the inner table's qualifier. If the alias differed from the table name, the resulting join condition referenced a column the rewritten subquery no longer exposed, so the query errored instead of returning rows.What changes are included in this PR?
When decorrelation pulls a correlated filter up through a
SubqueryAlias, the filter's column references are requalified to the alias, so the join condition matches the subquery's output schema.What is the testing strategy for this PR?
New cases in
lateral_join.sltcover a correlated LATERAL filter through a derived-table alias that differs from the table name. They fail onmainand pass with this change. Existing optimizer tests pass, andcargo fmtandcargo clippyare clean.Are there any user-facing changes?
Queries of this shape now plan and return results instead of erroring. No public API changes.
This change was made with help from Cursor (Grok). I reviewed and tested it.