[GitHub] [arrow] alamb commented on a change in pull request #8751: ARROW-10584: [Rust] [DataFusion] Add SQL support for JOIN ON syntax

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #8751:
URL: https://github.com/apache/arrow/pull/8751#discussion_r530957868



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -628,6 +701,53 @@ impl<'a, S: SchemaProvider> SqlToRel<'a, S> {
     }
 }
 
+fn create_join_schema(left: &SchemaRef, right: &SchemaRef) -> Result<Schema> {
+    let mut fields = vec![];
+    for field in left.fields() {
+        fields.push(field.clone());
+    }
+    for field in right.fields() {
+        fields.push(field.clone());
+    }
+    Ok(Schema::new(fields))
+}
+
+/// Parse equijoin ON condition which could be a single Eq or multiple conjunctive Eqs
+///
+/// Examples
+///
+/// foo = bar
+/// foo = bar AND bar = baz AND ...
+///
+fn extract_join_keys(expr: &Expr, accum: &mut Vec<(String, String)>) -> Result<()> {
+    match expr {

Review comment:
       👍 
   
   When I was reading this I have the feeling that the general pattern of "take an expression tree and break it into a list of conuncts (aka things that are AND'd together) will be a common one as we split filters up and push them around. 
   
   No action required here, just simply an observation

##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -1011,6 +1011,65 @@ fn create_case_context() -> Result<ExecutionContext> {
     Ok(ctx)
 }
 
+#[tokio::test]
+async fn equijoin() -> Result<()> {
+    let mut ctx = create_join_context()?;
+    let sql =
+        "SELECT t1_id, t1_name, t2_name FROM t1 JOIN t2 ON t1_id = t2_id ORDER BY t1_id";

Review comment:
       🎉 




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

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