ARROW-10669: [C++][Compute] Support scalar arguments to Boolean compute functions #8728
Conversation
bkietz
commented
Nov 20, 2020
- "and", "or", "xor", "and_kleene", and "or_kleene" gain support for scalar arguments.
- Added new functions "and_not" and "and_not_kleene"
- repaired and simplified some null propagation logic
pitrou
left a comment
There was a problem hiding this comment.
Thank you. Here are some comments. Also, can you update the doc in docs/source/cpp/compute.rst?
There was a problem hiding this comment.
Are you running almost the same tests twice here?
There was a problem hiding this comment.
No, the second tests exclusively array, scalar and scalar, array which isn't covered by CheckScalarBinary
There was a problem hiding this comment.
No, I mean you test a set of left / right inputs above, and another set of inputs here. Is there a reason to them separately? (just trying to understand if there is a particular reason...)
There was a problem hiding this comment.
Ah, I see. Yes they're the same input values (the full truth table) just with a the different call signatures
There was a problem hiding this comment.
Mention that heterogenous implementation ((scalar, array) and (array, scalar)) must broadcast the scalar value?
There was a problem hiding this comment.
That's implicit in the contract of ScalarFunctions though
cpp/src/arrow/compute/exec.cc
Outdated
There was a problem hiding this comment.
Would it be useful to add a SliceBitmap helper to bitmap_ops.h?
There was a problem hiding this comment.
Sounds worthwhile but I'd prefer to do it in follow up
There was a problem hiding this comment.
Hmm... Perhaps we can avoid a copy in most cases and just re-use the bitmap buffer?
(do we need a BitmapSliceOrCopy helper?)
There was a problem hiding this comment.
Reusing the bitmap like this will make it harder to refactor Kleene kernels to support writing into slices, which (I'm guessing) would be the greater performance win. In any case, it can wait till follow up
e7dd732 to
8f8c327
Compare