Re: [PR] fix: Correct ANY/ALL results for unaliased subqueries on the same table [datafusion]

["jayzhan211 (via GitHub)" <gi...@apache.org>]

jayzhan211 commented on code in PR #25273:
URL: https://github.com/apache/datafusion/pull/25273#discussion_r4358256659


##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -968,6 +968,60 @@ fn collect_local_correlated_cols(
     }
 }
 
+/// Extracts correlated predicates, such as a comparison between a column from the
+/// subquery and a column from the outer scope.
+///
+/// Preserves [`Expr::OuterReferenceColumn`] markers so callers can distinguish
+/// inner and outer columns when qualifying join filters (see [`build_join_filter`]).
+///
+/// # Arguments
+///
+/// * `exprs` - Subquery filter predicates to classify.
+///
+/// # Return value
+///
+/// Tuple of (correlated join filters, remaining subquery filters). Correlated
+/// self-equalities are discarded from both lists.
+pub(crate) fn find_join_exprs(exprs: Vec<&Expr>) -> (Vec<Expr>, Vec<Expr>) {
+    let mut joins = vec![];
+    let mut others = vec![];
+    for filter in exprs {
+        // Predicates containing outer references become join filters.
+        if filter.contains_outer() {
+            // Check equality before stripping markers: inner and outer columns

Review Comment:
   nit: this function doesn't remove the markers any more, so "before stripping markers" reads a little oddly here. Maybe: "Compare with the outer-reference markers intact: inner and outer columns with the same qualified name belong to different scopes."
   



##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -984,6 +1038,7 @@ fn remove_duplicated_filter(
 
     Ok(filters
         .into_iter()
+        .map(strip_outer_reference)

Review Comment:
   nit: this removes the outer-reference markers from the filters that are kept, not only for the comparison. So on the `IN` path they're gone again before `build_join_filter` gets to rename to the alias, which is the same issue this PR fixes. SQL can't hit it because inner columns shadow outer ones, but a hand-built plan can. Removing the markers from a copy keeps them intact, and the optimizer tests and `subquery*.slt` still pass with this:
   
   ```diff
        Ok(filters
            .into_iter()
   -        .map(strip_outer_reference)
            .filter(|filter| {
   +            let filter = &strip_outer_reference(filter.clone());
                if filter == in_predicate {
   ```
   



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org