-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
ARROW-10640: [C++] An "if_else" kernel to combine two arrays based on a mask #10410
Conversation
@bkietz I think the bitmap ops approach is simpler than the bitmap visitor approach. WDYT?
bitmap visitor -
|
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 looks good! I left some general comments.
As for visitor vs bitmap ops, I think the visitor-based one is fine. It does break down the cases nicely, even if it's a bit verbose.
output->buffers[0]->mutable_data()); | ||
break; | ||
case COND_ALL_VALID: // = 1 | ||
// out_valid = 0 --> nothing to do; but requires out_valid to be a all-zero buffer |
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.
AllocateBitmap doesn't zero the allocation
arrow/cpp/src/arrow/compute/kernel.h
Lines 61 to 64 in 1769888
/// \brief Allocate buffer for bitmap from the context's memory pool. Like | |
/// Allocate, the contents of the buffer are not initialized but the last | |
/// byte is preemptively zeroed to help avoid ASAN or valgrind issues. | |
Result<std::shared_ptr<ResizableBuffer>> AllocateBitmap(int64_t num_bits); |
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 think there is a mismatch in the kernel comment and the impl.
arrow/cpp/src/arrow/compute/kernel.cc
Line 56 in 29130ca
// Since bitmaps are typically written bit by bit, we could leak uninitialized bits. |
It looks like it is zeroed out for bitmaps
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.
Hmm, we should probably update comment then. CC @bkietz given KernelContext::AllocateBitmap has to zero the allocation to satisfy Valgrind et al, should we just go ahead and guarantee that it returns a pre-cleared bitmap?
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'd prefer to leave this for a follow up where we can try to zero only the final byte (as described by the comment) and see if Valgrind is appeased. If not, the comment can be updated
// out_buf = ones | ||
ARROW_ASSIGN_OR_RAISE(out_buf, ctx->AllocateBitmap(cond.length)); | ||
// filling with UINT8_MAX upto the buffer's size (in bytes) | ||
arrow::compute::internal::SetMemory<UINT8_MAX>(out_buf.get()); |
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.
You could just inline the memset call here - no need to put a helper in a header IMO unless we're going to use it a lot.
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.
Please inline the call to memset here
@github-actions autotune |
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.
just two more improvements
// out_buf = ones | ||
ARROW_ASSIGN_OR_RAISE(out_buf, ctx->AllocateBitmap(cond.length)); | ||
// filling with UINT8_MAX upto the buffer's size (in bytes) | ||
arrow::compute::internal::SetMemory<UINT8_MAX>(out_buf.get()); |
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.
Please inline the call to memset here
I did a quick comparison with
|
We may be able to further improve this if we directly use vector operations inside the kernel (I haven't checked the compiled code yet, may be compiler does that already), because if-else use case directly map to |
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
6208b36
to
4dbe076
Compare
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.
Excellent work, thanks for doing this!
It looks like there's an actual build failure on CentOS/GCC4.8 (expand "Dump install logs" to get the actual failure):
|
Ah! thanks @lidavidm I missed this... it seems to be a gcc 4.x thing, isn't it. BTW @bkietz should I add string type support to this as well, or can I leave it for later development? |
I think it is important to add support for strings as well, but it's probably a good idea to leave that for a follow-up PR so we can get this merged? (I can imagine the implementation to be a bit different / more complex for variable length array elements) |
I agree, and also Niranda already filed ARROW-12955 to take care of that followup. |
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 like that fixed the builds, and the remaining failures are not relevant.
…s based on a mask Adding a preliminary impl for an `if_else(cond: Datum, left: Datum, right: Datum)` function. It works as follows, ```python def if_else(cond, left, right): for c, true_val, false_val in zip(cond, left, right): if c: yield true_val else: yield false_val ``` `null` values will be promoted to the output. Closes apache#10410 from nirandaperera/ARROW-10640 Authored-by: niranda perera <niranda.perera@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Adding a preliminary impl for an
if_else(cond: Datum, left: Datum, right: Datum)
function. It works as follows,null
values will be promoted to the output.