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.

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

feat(expr): Implementation for bitwise operation #2884

Merged
merged 57 commits into from
Jun 3, 2022

Conversation

marvenlee2486
Copy link
Contributor

@marvenlee2486 marvenlee2486 commented May 28, 2022

What's changed and what's your intention?

Implement bitwise operation (& , | , # (XOR), ~, >> , << ) supports integer number.
Add unit testing for the expr
Add e2e testing

Refer to a related PR or issue link (optional)

closes #2494

@CLAassistant
Copy link

CLAassistant commented May 28, 2022

CLA assistant check
All committers have signed the CLA.

@lmatz
Copy link
Contributor

lmatz commented May 28, 2022

LGTM! Please resolve the conflicts later

@lmatz
Copy link
Contributor

lmatz commented May 28, 2022

And also rebase on main so the buildkite can be successfully triggered.

@marvenlee2486
Copy link
Contributor Author

May I know how to rebase on the main so that it can be trigger?

@lmatz
Copy link
Contributor

lmatz commented May 29, 2022

May I know how to rebase on the main so that it can be trigger?

On your local main branch, git fetch && git rebase origin/main. Switch to this branch at local, git checkout zongyu/bitwise_operation. And then rebase on main, git rebase main. And finally, git push -f origin.

@lmatz lmatz changed the title closes #2494 Implementation for bitwise operation feat(expr): Implementation for bitwise operation May 29, 2022
@lmatz
Copy link
Contributor

lmatz commented May 29, 2022

Run cargo fmt --all to pass the format checker 😄

@marvenlee2486 marvenlee2486 force-pushed the zongyu/bitwise_operation branch from ac7b8fb to d38b82c Compare May 29, 2022 18:13
@codecov
Copy link

codecov bot commented May 29, 2022

Codecov Report

Merging #2884 (481e578) into main (06f872d) will increase coverage by 0.02%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##             main    #2884      +/-   ##
==========================================
+ Coverage   72.62%   72.64%   +0.02%     
==========================================
  Files         718      719       +1     
  Lines       95936    96096     +160     
==========================================
+ Hits        69669    69806     +137     
- Misses      26267    26290      +23     
Flag Coverage Δ
rust 72.64% <85.18%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/expr/src/expr/expr_binary_nonnull.rs 84.28% <0.00%> (-1.44%) ⬇️
src/expr/src/expr/expr_unary.rs 75.50% <0.00%> (-0.44%) ⬇️
src/expr/src/lib.rs 100.00% <ø> (ø)
src/frontend/src/binder/expr/binary_op.rs 79.48% <0.00%> (-11.69%) ⬇️
src/frontend/src/binder/expr/mod.rs 84.05% <0.00%> (-0.29%) ⬇️
src/frontend/src/expr/function_call.rs 96.38% <0.00%> (-3.00%) ⬇️
src/expr/src/vector_op/bitwise_op.rs 91.93% <91.93%> (ø)
src/expr/src/vector_op/tests.rs 99.56% <96.15%> (-0.44%) ⬇️
src/expr/src/expr/mod.rs 48.88% <100.00%> (ø)
src/frontend/src/expr/type_inference.rs 99.00% <100.00%> (+0.09%) ⬆️
... and 5 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@lmatz lmatz force-pushed the zongyu/bitwise_operation branch from 4e5c88e to 3de3bc5 Compare May 30, 2022 02:54
{ int32, int16, int32, $general_f },
{ int32, int32, int32, $general_f },
{ int32, int64, int64, $general_f },
{ int64, int16,int64, $general_f },
Copy link
Contributor

Choose a reason for hiding this comment

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

Please reformat it, cargo fmt --all seems not work on code in macro.

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

Rest LGTM

src/expr/src/expr/expr_binary_nonnull.rs Outdated Show resolved Hide resolved
@lmatz lmatz requested a review from xiangjinwu May 30, 2022 03:39
e2e_test/test.slt Outdated Show resolved Hide resolved
proto/expr.proto Outdated Show resolved Hide resolved
src/expr/src/expr/expr_binary_nonnull.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_unary.rs Outdated Show resolved Hide resolved
src/frontend/src/expr/type_inference.rs Outdated Show resolved Hide resolved
src/expr/src/vector_op/bitwise_op.rs Outdated Show resolved Hide resolved
@xiangjinwu xiangjinwu added the user-facing-changes Contains changes that are visible to users label May 30, 2022
@marvenlee2486 marvenlee2486 force-pushed the zongyu/bitwise_operation branch from 06328bd to b7f0d46 Compare June 2, 2022 03:13
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 817 files.

Valid Invalid Ignored Fixed
815 1 1 0
Click to see the invalid file list
  • src/frontend/test_runner/tests/gen/testcases.rs

src/frontend/test_runner/tests/gen/testcases.rs Outdated Show resolved Hide resolved
@marvenlee2486
Copy link
Contributor Author

I think I have done the edition already, May I request for approver.

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

Good job!🚀

src/frontend/src/expr/type_inference.rs Outdated Show resolved Hide resolved
src/frontend/src/expr/type_inference.rs Outdated Show resolved Hide resolved
src/expr/src/expr/expr_binary_nonnull.rs Outdated Show resolved Hide resolved
src/expr/src/vector_op/bitwise_op.rs Outdated Show resolved Hide resolved
Comment on lines 34 to 37
general_shift(l, r, |a, b| match a.checked_shl(b) {
Some(c) => Ok(c),
None => Err(RwError::from(NumericValueOutOfRange)),
})
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI a.checked_shl(b).ok_or_else
Not sure if this looks better or worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I change it for mine part. general_shift(l, r, |a,b| a.checked_shl(b).ok_or_else( || RwError::from(NumericValueOutOfRange))). I feel it looks better.

Just to point out, the previous writing I was copying from arithmetic_op.rs. Should it be changed as well for consistency across similar code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping them consistent would be better.

src/expr/src/vector_op/tests.rs Show resolved Hide resolved
@marvenlee2486 marvenlee2486 requested a review from xiangjinwu June 3, 2022 08:10
@marvenlee2486 marvenlee2486 merged commit 165198c into main Jun 3, 2022
@marvenlee2486 marvenlee2486 deleted the zongyu/bitwise_operation branch June 3, 2022 08:15
@marvenlee2486 marvenlee2486 restored the zongyu/bitwise_operation branch June 3, 2022 08:44
Graphcalibur pushed a commit that referenced this pull request Jun 3, 2022
* Add bitwise_op.rs and implement << and >>

* Add ExprType and Do binding of BinaryOperator to ExprType

* ModifeModified expr and frontend

* Working for >> and <<

* Trying to add bitand but facing problem

* Done implementation for bitwise and or XOR(#), TODO ~ and Unit Test and e2e test

* Edit Bitwise not But still have server closed unexpected error

* Bitwise not done implemetation. TODO Testing

* Done Unit testing, and done code e2e test

* Done Unit testing, and done code e2e test

* last commit

* Final commit

* Edit minor changes

* Final commit

* return the error

* Revert

* Change to Upper Snake Case

* Remove wRemove extra white space at the end of the code

* Remove wadd extra ace at the end of the code

* Modifed e2e tests

* Delete Bitwise e2e

* Change e2e bitwise from select to values

* Revert "Change e2e bitwise from select to values"

This reverts commit 8934877.

* revision

* Edit error

* Resolve all comments except 'no overflow' comment

* Delete the additional test and solve formatting issues

* Resolve test error

* OTrucated to zero

* Change error in test

* Change error in test

* Edit Test error

* Edit test error

* Change format

* Resolve compilation error

* Resolve error

* Resolve error

* Resolve test error

* Resolve test error

* Resolve type_inference error

* Resolve format error

* Resolve format error

* Resolve format error

* Resolve test error

* Resolve error

* Resolve license fail

* Edit the lose edition

* Resolve error due to merge conflict resolve error

* Revert to having overflow error

* Remove autogenerated unwanted file

* Resolve some comments - Duplication, Docs on Code

* Resolves all other comments

* Delete unwanted import

* Resolve merge error

* Format error

Co-authored-by: LIU Zhi <lmatz823@gmail.com>
marvenlee2486 added a commit that referenced this pull request Jun 3, 2022
…2975)

Edit arithmetic.rs so that the format is consistent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement bit-wise operators & | # << >> ~ for integer operands
4 participants