ARROW-12955: [C++] Add additional type support for if_else kernel #10538
Conversation
lidavidm
left a comment
There was a problem hiding this comment.
This looks good on a first pass. I suggested some minor simplifications.
c046e4a to
f3b2258
Compare
There was a problem hiding this comment.
IMO these cases could be consolidated into one or two checks since there are only 12 cases here (or 5 if you want to prune: null cond, true cond with null/non-null left array, false cond with null/non-null right array).
There was a problem hiding this comment.
well, there are 64 cases TBH. Each param can be null/ non-null and scalar/ array (4 variations). Since we have 3 params we have 4x4x4 = 64 permutations. 🙂
There was a problem hiding this comment.
CheckWithDifferentShapes actually checks for all those cases + with offsets. But I agree, in CheckWithDifferentShapes it could check similar cases redundantly in several iterations. But we decided to leave it as it is.
There was a problem hiding this comment.
minor nit but I wonder if the compiler could generate similar (enough) code from a
struct CopyBinaryBulk {
const offset_type* offsets;
const uint8_t* values;
offset_type* out_offsets;
uint8_t* out_values;
operator()(...) { ... }
};
so you don't have to copy-paste these everywhere
|
N.B. it looks like this segfaults in the tests on Windows MSVC 2019 (https://github.com/apache/arrow/pull/10538/checks?check_run_id=2966481230#step:8:339) and there are various errors on Clang, MSVC, and MinGW (mostly related to implicit casts or needing to explicitly parameterize std::max). |
There was a problem hiding this comment.
maybe checked_cast<const FixedSizeBinaryType&>(*left.type()) or checked_pointer_cast<FixedSizeBinaryType>(left.type()) (will be dynamic cast in debug, static cast in release)
|
Looks like this also needs to be rebased now. But once that's done and the nits above are addressed I think this can be merged. |
…hods inside annon ns
0681ec3 to
ad17b68
Compare
# Conflicts: # cpp/src/arrow/compute/kernels/scalar_if_else.cc # cpp/src/arrow/compute/kernels/scalar_if_else_benchmark.cc # cpp/src/arrow/compute/kernels/scalar_if_else_test.cc
This PR adds fixed and variable binary type support