Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: incorrect nullability of between expr #6786

Merged
merged 1 commit into from
Jun 29, 2023
Merged

fix: incorrect nullability of between expr #6786

merged 1 commit into from
Jun 29, 2023

Conversation

jonahgao
Copy link
Member

Which issue does this PR close?

None

Rationale for this change

The nullability of the between expression not only depends on the subexpression but may also depend on the low or high expression.

For example

DataFusion CLI v27.0.0

❯ select 1 between null and 2;
+------------------------------------+
| Int64(1) BETWEEN NULL AND Int64(2) |
+------------------------------------+
|                                    |
+------------------------------------+

❯ select 1 between 1 and null;
+------------------------------------+
| Int64(1) BETWEEN Int64(1) AND NULL |
+------------------------------------+
|                                    |
+------------------------------------+

What changes are included in this PR?

fix incorrect nullability of between expr

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Jun 28, 2023
alamb
alamb approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jonahgao - this makes sense to me

datafusion/expr/src/expr_schema.rs

Expr::Between(Between {
expr, low, high, ..
}) => Ok(expr.nullable(input_schema)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

jackwener
jackwener approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thank you @jonahgao

@jackwener jackwener merged commit 06e22a5 into apache:main Jun 29, 2023
@jonahgao jonahgao deleted the between_nullability branch June 29, 2023 04:32
@jonahgao jonahgao mentioned this pull request Jun 29, 2023
Merged
yukkit pushed a commit to cnosdb/arrow-datafusion that referenced this pull request Jul 5, 2023
@jonahgao @yukkit
fix: incorrect nullability of between expr (apache#6786)
566e551
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@jackwener jackwener jackwener approved these changes

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jonahgao @alamb @jackwener