-
Notifications
You must be signed in to change notification settings - Fork 721
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
add lt_bool, lt_eq_bool, gt_bool, gt_eq_bool #860
Conversation
74503fb
to
2903c7d
Compare
Looks good! I think we should add the scalar versions too (lt_bool_scalar, etc)? |
i don't think we need.
|
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
==========================================
- Coverage 82.67% 82.66% -0.01%
==========================================
Files 168 168
Lines 48129 48172 +43
==========================================
+ Hits 39792 39823 +31
- Misses 8337 8349 +12
Continue to review full report at Codecov.
|
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.
After this PR, we will perhaps need to update eq_dyn
and the other kernels introduced in #858 to support BooleanArray
as well
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.
👨🍳 ❤️ -- very nice @jimexist -- thank you
/// Perform `left == right` operation on [`BooleanArray`] | ||
fn eq_bool(left: &BooleanArray, right: &BooleanArray) -> Result<BooleanArray> { | ||
#[inline] | ||
fn binary_boolean_op<F>( |
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 it worth a doc comment that says that op
is performed 64 bits at a time withu64
(aka so it is not possible to use a < b
for example, to implement lt_bool
)
|
||
/// Perform `left < right` operation on [`BooleanArray`] | ||
fn lt_bool(left: &BooleanArray, right: &BooleanArray) -> Result<BooleanArray> { | ||
binary_boolean_op(left, right, |a, b| ((!a) & b)) |
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.
👍
the truth table shows it to be true:
a | b | a < b |
---|---|---|
t | f | f |
t | f | f |
f | t | t |
f | t | f |
I checked the others as well
|
||
/// Perform `left <= right` operation on [`BooleanArray`] | ||
fn lt_eq_bool(left: &BooleanArray, right: &BooleanArray) -> Result<BooleanArray> { | ||
binary_boolean_op(left, right, |a, b| !(a & (!b))) |
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.
This can also be done with two operations rather than three via (!a | b)
but I suspect the compiler will do that transformation if it is useful
|
||
/// Perform `left >= right` operation on [`BooleanArray`] | ||
fn gt_eq_bool(left: &BooleanArray, right: &BooleanArray) -> Result<BooleanArray> { | ||
binary_boolean_op(left, right, |a, b| !((!a) & b)) |
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.
likewise could also be written like (a | !b)
Which issue does this PR close?
Following #844
Closes #861
Rationale for this change
add lt_bool, lt_eq_bool, gt_bool, gt_eq_bool
What changes are included in this PR?
Are there any user-facing changes?