infer right side nullability for LEFT join #5748

comphead · 2023-03-27T04:41:21Z

Which issue does this PR close?

Closes partially #5747 (LEFT Join Mismatch between schema and batches on a CREATE TABLE with a windowing query #5695 (comment)).

Rationale for this change

Weakens the schema comparison between planner schema and batches leaving only name and datatype to avoid false positives on checking not null and nullable columns

What changes are included in this PR?

Are these changes tested?

yes

Are there any user-facing changes?

milevin · 2023-03-27T16:40:11Z

@comphead I am not sure about this change; it doesn't seem sound.

The analyzer is assigning incorrect nullability flags to the right side of the join. I wonder if we shouldn't fix this instead.

comphead · 2023-03-27T16:46:28Z

I think its not that easy for analyzer to identify correct nullable flags without scanning entire data. for LEFT join it assumes right side cannot be matched and sets nullable and this sounds correct for me.

From other side we also have similar weaken check in optimizer https://github.com/apache/arrow-datafusion/blob/26b8377b0690916deacf401097d688699026b8fb/datafusion/optimizer/src/optimizer.rs#L423

milevin · 2023-03-27T16:54:53Z

Consider the above query. After plan_from_tables is finished in select.rs, the assigned column properties are as follows:

        DFField {
            qualifier: Some(
                Bare {
                    table: "t1",
                },
            ),
            field: Field {
                name: "col1",
                data_type: Int64,
                nullable: false,
                dict_id: 0,
                dict_is_ordered: false,
                metadata: {},
            },
        },
        DFField {
            qualifier: Some(
                Bare {
                    table: "t1",
                },
            ),
            field: Field {
                name: "col2",
                data_type: Utf8,
                nullable: false,
                dict_id: 0,
                dict_is_ordered: false,
                metadata: {},
            },
        },
        DFField {
            qualifier: Some(
                Bare {
                    table: "t2",
                },
            ),
            field: Field {
                name: "col3",
                data_type: Int64,
                nullable: false,
                dict_id: 0,
                dict_is_ordered: false,
                metadata: {},
            },
        },
        DFField {
            qualifier: Some(
                Bare {
                    table: "t2",
                },
            ),
            field: Field {
                name: "col4",
                data_type: Utf8,
                nullable: false,
                dict_id: 0,
                dict_is_ordered: false,
                metadata: {},
            },
        },

The two t2 fields have incorrect nullability flag inferred. Unless I am missing something, the plan_from_tables method does not need to scan the data to infer the correct nullability flags. All it needs is the understanding of which kind of join it is processing and which side of that join the columns are coming from.

Does it make sense?

comphead · 2023-03-27T17:00:16Z

yes, good catch, col4 has to have a true nullability. That might happen because optimizer sees literals and assumes they cannot be nulls, which will be incorrect in case of left join for sure. Checking this

milevin · 2023-03-27T17:13:01Z

This is before optimizer I believe. It's just the initial stage of building the logical plan from the query.

milevin · 2023-03-28T05:13:30Z

This is great @comphead! Check out the comments above.

comphead · 2023-03-28T14:40:30Z

datafusion/expr/src/logical_plan/builder.rs

+                .cloned()
+                .collect()
+        }
+        JoinType::Left => {


I'll fix other types of joins on separate PR

comphead · 2023-03-28T15:18:45Z

cool, @milevin @alamb @jackwener please review whenever you have time

milevin · 2023-03-28T16:50:22Z

cool, @milevin @alamb @jackwener please review whenever you have time

I'll approve, but can you address the question about equivalent_names_and_types still remaining in this PR?

milevin · 2023-03-28T05:09:47Z

datafusion/expr/src/logical_plan/builder.rs

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


Right should be treated symmetrically opposite of Left.

Full should be nullified on both sides.

#5748 (comment)

milevin · 2023-03-28T05:10:51Z

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


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

#5748 (comment)

milevin · 2023-03-28T05:11:42Z

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 {


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?

@comphead is this change relevant here?

Thanks, done, Ive got a better idea how to refactor this method.

comphead · 2023-03-29T15:28:53Z

@alamb @jackwener please check this PR so I can follow up later on other join types

datafusion/expr/src/logical_plan/builder.rs

alamb

This change makes sense to me -- thank you @comphead

It is somewhat amazing that his wasn't caught by existing tests.

alamb · 2023-03-29T16:52:47Z

The analyzer is assigning incorrect nullability flags to the right side of the join. I wonder if we shouldn't fix this inst

I believe the story is that @comphead plans to fix this in a follow on PR. Is that accurate @comphead ?

Thank you for the review @milevin

milevin · 2023-03-29T17:15:38Z

The analyzer is assigning incorrect nullability flags to the right side of the join. I wonder if we shouldn't fix this inst

I believe the story is that @comphead plans to fix this in a follow on PR. Is that accurate @comphead ?

@alamb This comment of mine was on an original version of this PR which tried to address the issue by weakening the compatibility check; @comphead followed the suggestion in the comment to rewrite the fix by changing the analyzer instead -- so all is good!

Note: for the case of LEFT join, the right side must be nullified (that's what the comment talks about). A follow on PR will handle RIGHT and FULL OUTER joins where the left side will have to be adjusted as well.

-- Michael

comphead · 2023-03-29T17:32:01Z

The analyzer is assigning incorrect nullability flags to the right side of the join. I wonder if we shouldn't fix this inst

I believe the story is that @comphead plans to fix this in a follow on PR. Is that accurate @comphead ?

@alamb This comment of mine was on an original version of this PR which tried to address the issue by weakening the compatibility check; @comphead followed the suggestion in the comment to rewrite the fix by changing the analyzer instead -- so all is good!

Note: for the case of LEFT join, the right side must be nullified (that's what the comment talks about). A follow on PR will handle RIGHT and FULL OUTER joins where the left side will have to be adjusted as well.

-- Michael

Right, for RIGHT/FULL OUTER joins I will create a follow up PR. This one is too overloaded :)

alamb · 2023-03-29T21:40:43Z

Thanks all !

weaken schema check on mem table

d135f69

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 27, 2023

comphead marked this pull request as ready for review March 27, 2023 05:18

weaken schema check on mem table

1674be4

comphead marked this pull request as draft March 27, 2023 17:00

fix LEFT join column null inference

3302426

github-actions bot added the logical-expr Logical plan and expressions label Mar 28, 2023

comphead changed the title ~~weaken schema check on mem table~~ infer right side nullability for LEFT join Mar 28, 2023

comphead added 2 commits March 27, 2023 17:32

fix LEFT join column null inference. fix test

b34e1b5

fix LEFT join column null inference. fix test

bf8ed7b

github-actions bot added the optimizer Optimizer rules label Mar 28, 2023

comphead marked this pull request as ready for review March 28, 2023 14:39

comphead commented Mar 28, 2023

View reviewed changes

milevin approved these changes Mar 28, 2023

View reviewed changes

fix LEFT join column null inference. rollback some code

b8af34d

alamb reviewed Mar 29, 2023

View reviewed changes

datafusion/expr/src/logical_plan/builder.rs Show resolved Hide resolved

alamb approved these changes Mar 29, 2023

View reviewed changes

alamb merged commit f210cac into apache:main Mar 29, 2023

comphead mentioned this pull request Mar 29, 2023

[EPIC] Schema mismatches with batches schema #5747

Closed

6 tasks

comphead mentioned this pull request Apr 13, 2023

infer join side nullability for joined tables #5994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infer right side nullability for LEFT join #5748

infer right side nullability for LEFT join #5748

comphead commented Mar 27, 2023 •

edited

Loading

milevin commented Mar 27, 2023

comphead commented Mar 27, 2023

milevin commented Mar 27, 2023 •

edited

Loading

comphead commented Mar 27, 2023 •

edited

Loading

milevin commented Mar 27, 2023

milevin commented Mar 28, 2023 •

edited

Loading

comphead Mar 28, 2023

comphead commented Mar 28, 2023

milevin commented Mar 28, 2023

milevin Mar 28, 2023

comphead Mar 28, 2023

milevin Mar 28, 2023

comphead Mar 28, 2023

milevin Mar 28, 2023

milevin Mar 28, 2023

comphead Mar 28, 2023

comphead commented Mar 29, 2023

alamb left a comment

alamb commented Mar 29, 2023

milevin commented Mar 29, 2023 •

edited

Loading

comphead commented Mar 29, 2023

alamb commented Mar 29, 2023

infer right side nullability for LEFT join #5748

infer right side nullability for LEFT join #5748

Conversation

comphead commented Mar 27, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

milevin commented Mar 27, 2023

comphead commented Mar 27, 2023

milevin commented Mar 27, 2023 • edited Loading

comphead commented Mar 27, 2023 • edited Loading

milevin commented Mar 27, 2023

milevin commented Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

comphead commented Mar 28, 2023

milevin commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Mar 29, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb commented Mar 29, 2023

milevin commented Mar 29, 2023 • edited Loading

comphead commented Mar 29, 2023

alamb commented Mar 29, 2023

comphead commented Mar 27, 2023 •

edited

Loading

milevin commented Mar 27, 2023 •

edited

Loading

comphead commented Mar 27, 2023 •

edited

Loading

milevin commented Mar 28, 2023 •

edited

Loading

milevin commented Mar 29, 2023 •

edited

Loading