Implement boolean equality kernels by Dandandan · Pull Request #844 · apache/arrow-rs

Dandandan · 2021-10-21T21:05:51Z

Which issue does this PR close?

Closes #842

Rationale for this change

Implementing (fast) equality kernels for booleans.

What changes are included in this PR?

Implement eq_bool, neq_bool , eq_bool_scalar, neq_bool_scalar.

Dandandan · 2021-10-21T21:07:01Z

arrow/src/compute/kernels/comparison.rs

+                right,
+                right_offset_in_bits,
+                len_in_bits,
+                |a, b| !(a ^ b),


This works per pair of u64 instead of per bool.

Dandandan · 2021-10-21T21:09:26Z

arrow/src/compute/kernels/comparison.rs

+    };
+
+    let data = unsafe {
+        ArrayData::new_unchecked(


Not sure if there is a more elegant way of doing this.

Dandandan · 2021-10-21T21:17:50Z

We have some formatting differences in some tests it seems from a rust version upgrade (?).

Dandandan · 2021-10-21T21:39:34Z

Also some new (good, unrelated) clippy error.

alamb

I plan to review this in the morning. Thank you @Dandandan !

xudong963 · 2021-10-22T06:35:41Z

arrow/src/compute/kernels/comparison.rs

+         right: &Buffer,
+         right_offset_in_bits: usize,
+         len_in_bits: usize| {
+            bitwise_bin_op_helper(


How about using bitwise_bin_op_simd_helper?

That only should be used together with a simd feature / implementation.
Certainly possible, but could be implemented as follow up work.

alamb · 2021-10-22T10:08:31Z

arrow/src/compute/kernels/comparison.rs

+
+/// Perform `left != right` operation on [`BooleanArray`] and a scalar
+fn neq_bool_scalar(left: &BooleanArray, right: bool) -> Result<BooleanArray> {
+    let len = left.len();


I wonder if this could simply be a call to eq_bool_scalar(left, !right)?

alamb · 2021-10-22T10:13:27Z

arrow/src/compute/kernels/comparison.rs

    }

+    #[test]
+    fn test_boolean_array_eq() {


Normally since there is special case handling for processing these at 64 bits at a time, I would suggest tests that have at least 65 entries to test the boundary conditions. However, in this case as it is using a pre-existing helper method bitwise_bin_op_helper I am not sure such extra coverage is needed

alamb · 2021-10-22T10:14:27Z

Very cool -- thanks a lot @Dandandan

codecov-commenter · 2021-10-22T12:39:56Z

Codecov Report

Merging #844 (ecad119) into master (4cfe621) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
+ Coverage   82.64%   82.67%   +0.03%     
==========================================
  Files         168      168              
  Lines       48088    48129      +41     
==========================================
+ Hits        39742    39791      +49     
+ Misses       8346     8338       -8

Impacted Files	Coverage Δ
arrow/src/compute/kernels/boolean.rs	`96.81% <100.00%> (ø)`
arrow/src/compute/kernels/cast.rs	`94.62% <100.00%> (+0.01%)`	⬆️
arrow/src/compute/kernels/comparison.rs	`95.55% <100.00%> (+0.35%)`	⬆️
arrow/src/ipc/reader.rs	`86.08% <100.00%> (+1.33%)`	⬆️
parquet/src/schema/parser.rs	`86.37% <100.00%> (+0.09%)`	⬆️
arrow/src/array/equal_json.rs	`84.92% <0.00%> (-0.29%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd9d561...ecad119. Read the comment docs.

jhorstmann · 2021-10-22T21:56:41Z

Thanks! These look correct to me, including the null handling.

alamb · 2021-10-23T11:50:37Z

Thanks again @Dandandan

* Implement boolean equality kernels * Respect offset * Simplify

jimexist · 2021-10-25T01:38:59Z

@Dandandan @alamb i wonder if you think defining lt et. al. makes sense here, if we allow false < true. if that's defined then i can simplify this branch

alamb · 2021-10-25T10:46:32Z

@Dandandan @alamb i wonder if you think defining lt et. al. makes sense here, if we allow false < true. if that's defined then i can simplify this branch

I personally think it does make sense to define the other logical comparison operators for BooleanArray (even though the usecase for bool < bool is limited). My primary rationale is for completeness (SQL and Postgres allows for it) and I do think it will avoid having to special case boolean columns for features like DataFusion partition pruning (which converts = into <= and > comparisons for comparing to statistics)

Also, postgres supports it, FWIW

alamb=# select true < false;
 ?column? 
----------
 f
(1 row)

alamb=# select true > false;
 ?column? 
----------
 t
(1 row)

* Implement boolean equality kernels * Respect offset * Simplify Co-authored-by: Daniël Heres <[email protected]>

Dandandan · 2021-10-25T10:57:31Z

I allgree it makes sense to define lt etc. 👍

jimexist · 2021-10-25T11:31:50Z

I allgree it makes sense to define lt etc. 👍

in that case, after #858 I'll try to add it

jimexist · 2021-10-25T13:40:20Z

I allgree it makes sense to define lt etc. 👍

see #860

Implement boolean equality kernels

4763c24

Dandandan requested a review from alamb October 21, 2021 21:05

github-actions bot added the arrow Changes to the arrow crate label Oct 21, 2021

Dandandan commented Oct 21, 2021

View reviewed changes

Respect offset

1188299

Dandandan commented Oct 21, 2021

View reviewed changes

alamb reviewed Oct 21, 2021

View reviewed changes

xudong963 reviewed Oct 22, 2021

View reviewed changes

alamb approved these changes Oct 22, 2021

View reviewed changes

Dandandan added 2 commits October 22, 2021 14:24

Merge remote-tracking branch 'upstream/master' into bool_eq_methods

81381ae

Simplify

ecad119

alamb mentioned this pull request Oct 23, 2021

Support =, <, <=, >, >=, !=, is distinct from, is not distinct from for BooleanArray apache/datafusion#1163

Merged

alamb merged commit 3ea0e18 into apache:master Oct 23, 2021

alamb pushed a commit that referenced this pull request Oct 24, 2021

Implement boolean equality kernels (#844)

614ee88

* Implement boolean equality kernels * Respect offset * Simplify

alamb added the cherry-picked label Oct 24, 2021

alamb mentioned this pull request Oct 24, 2021

Cherry pick Implement boolean equality kernels to active_release #857

Merged

alamb added a commit that referenced this pull request Oct 25, 2021

Implement boolean equality kernels (#844) (#857)

620282a

* Implement boolean equality kernels * Respect offset * Simplify Co-authored-by: Daniël Heres <[email protected]>

This was referenced Oct 25, 2021

add lt_bool, lt_eq_bool, gt_bool, gt_eq_bool #860

Merged

Add boolean lt, gt, leq, geq kernels #861

Closed

This was referenced Nov 4, 2021

boolean comparison kernels are not pub (and can't be used) #912

Closed

Mark boolean comparison kernels public #913

Merged

This was referenced Nov 20, 2021

Add boolean comparison to scalar kernels for less then, greater than #959

Closed

Consider adding is_distinct_from kernels #960

Closed

Comments

Conversation

Dandandan commented Oct 21, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Uh oh!

Dandandan Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Oct 21, 2021

Uh oh!

Dandandan commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

xudong963 Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Oct 22, 2021

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 22, 2021

Uh oh!

codecov-commenter commented Oct 22, 2021

Codecov Report

Uh oh!

jhorstmann commented Oct 22, 2021

Uh oh!

alamb commented Oct 23, 2021

Uh oh!

jimexist commented Oct 25, 2021

Uh oh!

alamb commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dandandan commented Oct 25, 2021

Uh oh!

jimexist commented Oct 25, 2021

Uh oh!

jimexist commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Dandandan commented Oct 21, 2021 •

edited

Loading

alamb commented Oct 25, 2021 •

edited

Loading