Add subtrait support for IS NULL and IS NOT NULL
#8093
Conversation
alamb
changed the title
IS NULL and IS NOT NULL
| make_datafusion_like(true, f, input_schema, extensions).await | ||
| } | ||
| ScalarFunctionType::IsNull => { | ||
| let arg = f.arguments.first().ok_or_else(|| { |
There was a problem hiding this comment.
I don't know if it matters, but this code doesn't check for f.arguments.len() > 1 so I think it will silently ignore any arguments after the first.
The same comment applies to IsNotNull
There was a problem hiding this comment.
On review, this is the same pattern used elsewhere in this PR
There was a problem hiding this comment.
Thanks for merging my PR! I think I could add checks for arg length here and also in other places where they are required in another PR
There was a problem hiding this comment.
Thank you @tgujar -- A follow on to make the argument checking handle too many arguments would be most appreciated. Thank you 🙏
| }]; | ||
|
|
||
| let function_name = "is_null".to_string(); | ||
| let function_anchor = _register_function(function_name, extension_info); |
There was a problem hiding this comment.
Very minor, but why not call this function_reference to match the field name used below?
There was a problem hiding this comment.
While reviewing the code again, I found this simply follows the same pattern as the existing substrait code, so looks good to me
There was a problem hiding this comment.
@tgujar if you have time, it would also be awesome if you could make a PR that renames this variable (and other uses of _register_function to function_reference which I think would make the code cleaner
|
Thanks again @tgujar |
2 participants
Which issue does this PR close?
Closes #8087.
What changes are included in this PR?
Support for IS NULL and IS NOT NULL in Substrait producer and consumer. Also includes basic tests for this functionality.
Are these changes tested?
Yes
Are there any user-facing changes?
No