[GitHub] [arrow-datafusion] milevin commented on a diff in pull request #5748: infer right side nullability for LEFT join

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

milevin commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150033659


##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -1041,20 +1041,44 @@ pub fn build_join_schema(
     right: &DFSchema,
     join_type: &JoinType,
 ) -> Result<DFSchema> {
+    let right_fields = right.fields();
+    let left_fields = left.fields();
+
     let fields: Vec<DFField> = match join_type {
-        JoinType::Inner | JoinType::Left | JoinType::Full | JoinType::Right => {
-            let right_fields = right.fields().iter();
-            let left_fields = left.fields().iter();
+        JoinType::Inner | JoinType::Full | JoinType::Right => {

Review Comment:
   Right should be treated symmetrically opposite of Left.
   
   Full should be nullified on both sides.



##########
datafusion/common/src/utils.rs:
##########
@@ -234,6 +235,18 @@ pub(crate) fn parse_identifiers_normalized(s: &str) -> Vec<String> {
         .collect::<Vec<_>>()
 }
 
+/// Check two schemas for being equal for field names/types
+pub fn equivalent_names_and_types(schema: &SchemaRef, other: SchemaRef) -> bool {

Review Comment:
   Do you still need this change for equivalent_names_and_types? I don't think it's used in your solution to the problem; right?



##########
datafusion/core/tests/sqllogictests/test_files/join.slt:
##########
@@ -530,6 +530,13 @@ FROM t1
 ----
 11 a 55
 
+# test create table from query with LEFT join

Review Comment:
   It would be great to add tests like this for Right, Full, and all the other flavors of joins.



##########
datafusion/common/src/utils.rs:
##########
@@ -234,6 +235,18 @@ pub(crate) fn parse_identifiers_normalized(s: &str) -> Vec<String> {
         .collect::<Vec<_>>()
 }
 
+/// Check two schemas for being equal for field names/types
+pub fn equivalent_names_and_types(schema: &SchemaRef, other: SchemaRef) -> bool {

Review Comment:
   @comphead is this change relevant here?



-- 
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@arrow.apache.org

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