You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2023/03/27 04:41:24 UTC
[GitHub] [arrow-datafusion] comphead opened a new pull request, #5748: weaken schema check on mem table
comphead opened a new pull request, #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
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
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
yes
```
with t1 as (select 1 as col1, 'asd' as col2), t2 as (select 1 as col3, 'sdf' as col4)
select col2, col4 from t1 left join t2 on col1 = col3;
+------+------+
| col2 | col4 |
+------+------+
| asd | sdf |
+------+------+
1 row in set. Query took 0.019 seconds.
```
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: infer right side nullability for LEFT join
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1488998347
> > 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 OUTER join where the left side will have to be adjusted as well.
--
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
[GitHub] [arrow-datafusion] comphead commented on pull request #5748: weaken schema check on mem table
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1485476174
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
--
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
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150955041
##########
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:
Thanks, done, Ive got a better idea how to refactor this method.
--
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
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1152275743
##########
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 => {
// left then right
- left_fields.chain(right_fields).cloned().collect()
+ left_fields
+ .iter()
+ .chain(right_fields.iter())
+ .cloned()
+ .collect()
+ }
+ JoinType::Left => {
+ // left then right, right set to nullable in case of not matched scenario
+ let right_fields_nullable: Vec<DFField> = right_fields
+ .iter()
+ .map(|f| {
+ let field = f.field().clone().with_nullable(true);
+ if let Some(q) = f.qualifier() {
+ DFField::from_qualified(q, field)
+ } else {
+ DFField::from(field)
Review Comment:
That sounds great! I'll create another PR to extend API
--
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
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150732166
##########
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 => {
// left then right
- left_fields.chain(right_fields).cloned().collect()
+ left_fields
+ .iter()
+ .chain(right_fields.iter())
+ .cloned()
+ .collect()
+ }
+ JoinType::Left => {
Review Comment:
I'll fix other types of joins on separate PR
--
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
[GitHub] [arrow-datafusion] comphead commented on pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1489020087
> > > 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 :)
--
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
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150932624
##########
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:
https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150732166
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: weaken schema check on mem table
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1485529901
This is before optimizer I believe. It's just the initial stage of building the logical plan from the query.
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1152231567
##########
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 => {
// left then right
- left_fields.chain(right_fields).cloned().collect()
+ left_fields
+ .iter()
+ .chain(right_fields.iter())
+ .cloned()
+ .collect()
+ }
+ JoinType::Left => {
+ // left then right, right set to nullable in case of not matched scenario
+ let right_fields_nullable: Vec<DFField> = right_fields
+ .iter()
+ .map(|f| {
+ let field = f.field().clone().with_nullable(true);
+ if let Some(q) = f.qualifier() {
+ DFField::from_qualified(q, field)
+ } else {
+ DFField::from(field)
Review Comment:
Maybe we could add a `DFField::with_nullable` to make this more concise:
```
let field = f.clone().with_nullable(true);
```
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #5748: infer right side nullability for LEFT join
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: infer right side nullability for LEFT join
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1487275825
> 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?
--
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
[GitHub] [arrow-datafusion] comphead commented on pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1487092424
cool, @milevin @alamb @jackwener please review whenever you have time
--
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
[GitHub] [arrow-datafusion] comphead commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150932363
##########
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:
https://github.com/apache/arrow-datafusion/pull/5748#discussion_r1150732166
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: infer right side nullability for LEFT join
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1486227839
This is great! Check out the comments above.
--
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
[GitHub] [arrow-datafusion] comphead commented on pull request #5748: weaken schema check on mem table
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1485510993
yes, good catch, col4 has to have a true nullability
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #5748: infer right side nullability for LEFT join
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1489366161
Thanks all !
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: weaken schema check on mem table
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1485466916
@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.
--
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
[GitHub] [arrow-datafusion] milevin commented on pull request #5748: weaken schema check on mem table
Posted by "milevin (via GitHub)" <gi...@apache.org>.
milevin commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1485496016
Consider the above query. After `plan_selection` 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_selection` method does not need to scan the data to infer the correct nullability flags. All it needs is the understanding of which join it is processing and which side of that join the columns are coming from.
Does it make sense?
--
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
[GitHub] [arrow-datafusion] comphead commented on pull request #5748: infer right side nullability for LEFT join
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1488843783
@alamb @jackwener please check this PR so I can follow up later on other join types
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #5748: infer right side nullability for LEFT join
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5748:
URL: https://github.com/apache/arrow-datafusion/pull/5748#issuecomment-1488958456
> 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
--
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
[GitHub] [arrow-datafusion] milevin commented on a diff in pull request #5748: infer right side nullability for LEFT join
Posted by "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