-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Fix: support NULL input for regular expression comparison operations #11985
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @HuSen8891 for the contribution 🙏
Can you please add some end to end tests, in sqllogicteset format?
Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
Ideally you should be able to extend one of the existing test files in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files
Perhaps https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/regexp.slt
(DataType::Null, Utf8View | Utf8 | LargeUtf8) => Some(rhs_type.clone()), | ||
(Utf8View | Utf8 | LargeUtf8, DataType::Null) => Some(lhs_type.clone()), | ||
(DataType::Null, DataType::Null) => Some(Utf8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is any reason to explicitly list out the types?
Like maybe you could simplify this a bit like
let lhs_null = matches!(lhs_type, DataType::Null);
let rhs_null = matches!(rhs_type, DataType::Null);
math (lhs_null, rhs_null) {
(true, false) => Some(rhs_type.clone()),
(false, true) => Some(lhs_type.clone())
(true, true) => Some(Utf8)
(false, false) => None,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we only accept string and NULL input, not other type, such as select 'abc' ~ null;
But select 1 ~ null is not supported.
add some end to end tests in https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/regexp.slt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- thank you @HuSen8891
Thanks again @HuSen8891 |
Which issue does this PR close?
Related with #11623
Rationale for this change
Regular expression comparison operations accept NULL input, like
select 'abc' ~ null;
or
select null ~ null;
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?