Re: [PR] fix(sql): fix a bug when planning semi- or antijoins [datafusion]

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

neilconway commented on code in PR #20990:
URL: https://github.com/apache/datafusion/pull/20990#discussion_r2997303222


##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4847,6 +4847,40 @@ fn test_using_join_wildcard_schema() {
     );
 }
 
+#[test]
+fn test_using_join_wildcard_schema_semi_anti() {
+    let left_expected = vec!["s.x1".to_string(), "s.x2".to_string(), "s.x3".to_string()];
+    let right_expected = vec!["t.x1".to_string(), "t.x2".to_string(), "t.x3".to_string()];
+
+    let sql = "WITH 
+        s AS (SELECT 1 AS x1, 2 AS x2, 3 AS x3),
+        t AS (SELECT 1 AS x1, 4 AS x2, 5 AS x3)
+        SELECT * FROM s LEFT SEMI JOIN t USING (x1)";

Review Comment:
   This test (and the other `LEFT ANTI JOIN` test) should succeed without this PR, right? Because the correct side of the join happens to be picked. Can we change this (or add a new test variant) that checks that the behavior has changed for `LEFT` joins?



##########
datafusion/expr/src/utils.rs:
##########


Review Comment:
   It took me a while to wrap my head around the logic here; what do you think of this instead?
   
   ```suggestion
     /// When a JOIN has a USING clause, the join columns appear in the output
     /// schema once per side (for inner/outer joins) or once total (for semi/anti
     /// joins). An unqualified wildcard should include each USING column only once.
     /// This function returns the duplicate columns that should be excluded.
     fn exclude_using_columns(plan: &LogicalPlan) -> Result<HashSet<Column>> {
         let output_columns: HashSet<_> = plan.schema().columns().iter().cloned().collect();
         let mut excluded = HashSet::new();
   
         for cols in plan.using_columns()? {
             // `using_columns()` returns join columns from both sides regardless
             // of join type. For semi/anti joins, only one side's columns appear
             // in the output schema. Filter to output columns so that columns
             // from the non-output side don't participate in deduplication and
             // displace real output columns.
             let mut cols: Vec<_> = cols
                 .into_iter()
                 .filter(|c| output_columns.contains(c))
                 .collect();
                 
             // Sort so we keep the same qualified column, regardless of HashSet
             // iteration order.
             cols.sort();
   
             let mut seen = HashSet::new();
             for col in cols {
                 if !seen.insert(col.name.clone()) {
                     excluded.insert(col);
                 }
             }
         }
   
         Ok(excluded)
     }
   ```



##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -4847,6 +4847,40 @@ fn test_using_join_wildcard_schema() {
     );
 }
 
+#[test]
+fn test_using_join_wildcard_schema_semi_anti() {
+    let left_expected = vec!["s.x1".to_string(), "s.x2".to_string(), "s.x3".to_string()];
+    let right_expected = vec!["t.x1".to_string(), "t.x2".to_string(), "t.x3".to_string()];

Review Comment:
   ```suggestion
       let left_expected = &["s.x1", "s.x2", "s.x3"];
       let right_expected = &["t.x1", "t.x2", "t.x3"];
   ```



-- 
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