-
Notifications
You must be signed in to change notification settings - Fork 209
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
Feature/operator bit not #1321
Feature/operator bit not #1321
Conversation
Pull Request Test Coverage Report for Build 5658019652
💛 - Coveralls |
I am going to add some more tests to increase coverage. |
Evaluated::StrSlice { source, range } => { | ||
Err(EvaluateError::UnsupportedUnaryBitNot(source[range.clone()].to_owned()).into()) | ||
} |
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.
Can we also add test code for this case?
https://coveralls.io/builds/61510412/source?filename=core%2Fsrc%2Fexecutor%2Fevaluate%2Fevaluated.rs#L259
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.
Let me try.
Thank you for the review.
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 pushed a new patch to test EvaluateError::UnsupportedUnaryBitNot error.
This patch adds some test cases for the bitwise-not operator ~.
6bd1acd
to
c206b1c
Compare
This patch implements ~ operator - Add translation from SqlUnaryOperator::PGBitwiseNot to ast::UnaryOerator::BitNot - add ast::UnaryOperator::BitNot to ToSql implementation for UnaryOperator - Add evaluation of UnaryOperator::BitNot into Value enum - Add implementation of ~ operator in Value enum
This patch adds more tests to increase test coverage.
One more test to increase test coverage
c206b1c
to
eea4753
Compare
I will add a test case to cover Null data. |
The test missed ~ operator.
core/src/ast/operator.rs
Outdated
@@ -18,6 +19,7 @@ impl ToSql for UnaryOperator { | |||
UnaryOperator::Minus => "-".to_owned(), | |||
UnaryOperator::Not => "NOT ".to_owned(), | |||
UnaryOperator::Factorial => "!".to_owned(), | |||
&UnaryOperator::BitNot => "~".to_owned(), |
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.
&
is unnecessary. we can remove this.- could we use
Bitwise
prefix rather thanBit
because we already usebitwise
for other operators. e.g.BinaryOperator::BitwiseAnd
andBinaryOperator::BitwiseShiftLeft
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.
- Ok. It's my mistake.
- Ok. I will change it to BitwiseNot.
There are already other bitwise operators, for instance bitwise_and and bitwise_shift_left.
Hi, Now I think it's ready to be merged. |
core/src/executor/evaluate/expr.rs
Outdated
@@ -66,6 +66,7 @@ pub fn unary_op<'a>(op: &UnaryOperator, v: Evaluated<'a>) -> Result<Evaluated<'a | |||
UnaryOperator::Minus => v.unary_minus(), | |||
UnaryOperator::Not => v.try_into().map(|v: bool| Evaluated::from(Value::Bool(!v))), | |||
UnaryOperator::Factorial => v.unary_factorial(), | |||
&UnaryOperator::BitwiseNot => v.unary_bitwise_not(), |
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.
we can remove this &
, too.
core/src/translate/operator.rs
Outdated
@@ -13,6 +13,7 @@ pub fn translate_unary_operator(sql_unary_operator: &SqlUnaryOperator) -> Result | |||
SqlUnaryOperator::Minus => Ok(UnaryOperator::Minus), | |||
SqlUnaryOperator::Not => Ok(UnaryOperator::Not), | |||
SqlUnaryOperator::PGPostfixFactorial => Ok(UnaryOperator::Factorial), | |||
&SqlUnaryOperator::PGBitwiseNot => Ok(UnaryOperator::BitwiseNot), |
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, too.
34b5bd4
to
5c53141
Compare
02fd855
to
a8983b5
Compare
Hi,
|
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 all nice! Thanks a lot for the contribution 👍 👍
In the last PR, I pushed a implementation for bot of
BIT_NOT
function and ~ operator.As review comment, I removed implementation of
BIT_NOT
function and pushed only ~ operator in this PR.Please review.
I will add more test cases if the coverage test result is not good enough.