kosiew commented on code in PR #26044:
URL: https://github.com/apache/datafusion/pull/26044#discussion_r4356624031
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -657,22 +675,49 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr {
&self.correlated_subquery_cols_map,
&mut local_correlated_cols,
);
+ // A filter pulled up from below this alias still names the
+ // inner table (`l.id`). The alias's output names that column
+ // `l2.id`, and the join condition has to match.
+ // https://github.com/apache/datafusion/issues/25837
+ //
+ // Pull-up can add a column whose name another projected
+ // 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 {
Review 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.
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -1015,6 +1060,65 @@ fn can_pullup_over_aggregation(expr: &Expr) -> bool {
}
}
+/// Rewrite columns in `filters` that are listed in `cols` so they use the
+/// alias's output column at the same position.
+///
+/// `cols` are the correlated columns as named below the alias. Outer
+/// references are not in that set, so they stay as they are. The caller
+/// passes only the filters added under this alias.
+///
+/// `input_schema` is the schema passed to `SubqueryAlias::try_new`.
+/// `output_schema` is the schema it built. Fields line up by position,
+/// including the dedup suffixes `try_new` applies.
+fn requalify_join_filter_columns(
+ filters: &mut [Expr],
+ cols: &BTreeSet<Column>,
+ input_schema: &DFSchema,
+ output_schema: &DFSchema,
+) -> Result<()> {
+ if cols.is_empty() || filters.is_empty() {
+ return Ok(());
+ }
+ for filter in filters {
+ *filter = filter
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
