-
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
Implement unparse IS_NULL
to String and enhance the tests
#10529
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 for the contribution @goldmedal -- this code looks like a great first contribution.
I just kicked off the CI checks and once we get them to pass successfully I'll merge this PR in
Expr::IsNull(_) => not_impl_err!("Unsupported Expr conversion: {expr:?}"), | ||
Expr::IsNull(expr) => { | ||
Ok(ast::Expr::IsNull(Box::new(self.expr_to_sql(expr)?))) | ||
} | ||
Expr::IsNotFalse(_) => not_impl_err!("Unsupported Expr conversion: {expr:?}"), |
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.
Ah it looks like I maybe mis read the code originally and the other missing expression is Expr::IsNotFalse
(rather than Expr::IsNotNull
)
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.
Thanks for reviewing! I think I can fix IsNotFalse
in another PR. What do you think?
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 think a second PR will be great
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.
LGTM.
You need to run 'cargo fmt' to format the code,
also I think these additional test cases may not be necessary but it's good to keep them.
Thanks again @goldmedal -- welcome to the project! |
…0529) * implement unparse is_null and add test * format the code
Which issue does this PR close?
Closes #10519
Rationale for this change
What changes are included in this PR?
I found that
is_not_null
has been supported. I only implement 'is_null` here.Are these changes tested?
Yes, I also added more test cases for
is_null
andis_not_null
(e.g. function call and case when).Are there any user-facing changes?
No