ARROW-18342: [C++] AsofJoinNode support for Boolean data field#14658
ARROW-18342: [C++] AsofJoinNode support for Boolean data field#14658westonpace merged 2 commits intoapache:masterfrom
Conversation
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
I have two suggestions, but looks good so far.
| template <class Type, class Builder = typename TypeTraits<Type>::BuilderType> | ||
| enable_if_t<is_fixed_width_type<Type>::value && !is_boolean_type<Type>::value, | ||
| Status> static BuilderAppend(Builder& builder, | ||
| const std::shared_ptr<ArrayData>& source, | ||
| row_index_t row) { | ||
| if (source->IsNull(row)) { | ||
| builder.UnsafeAppendNull(); | ||
| return Status::OK(); | ||
| } |
There was a problem hiding this comment.
One pattern we've been moving to in the code base is using constexpr conditions:
| template <class Type, class Builder = typename TypeTraits<Type>::BuilderType> | |
| enable_if_t<is_fixed_width_type<Type>::value && !is_boolean_type<Type>::value, | |
| Status> static BuilderAppend(Builder& builder, | |
| const std::shared_ptr<ArrayData>& source, | |
| row_index_t row) { | |
| if (source->IsNull(row)) { | |
| builder.UnsafeAppendNull(); | |
| return Status::OK(); | |
| } | |
| template <class Type, class Builder = typename TypeTraits<Type>::BuilderType> | |
| enable_if_fixed_width_type<Type, Status> static BuilderAppend(Builder& builder, | |
| const std::shared_ptr<ArrayData>& source, | |
| row_index_t row) { | |
| if (source->IsNull(row)) { | |
| builder.UnsafeAppendNull(); | |
| return Status::OK(); | |
| } | |
| if constexpr (is_boolean_type<Type>::value) { | |
| builder.UnsafeAppend(bit_util::GetBit(source->template GetValues<uint8_t>(1), row)); | |
| } else { | |
| using CType = typename TypeTraits<Type>::CType; | |
| builder.UnsafeAppend(source->template GetValues<CType>(1)[row]); | |
| } |
(Note this suggestion is incomplete, since GitHub limits which lines I can diff.)
| batches.schema = schema; | ||
| int n_fields = schema->num_fields(); | ||
| for (auto num_batch : num_batches.batches) { | ||
| Datum two(ConstantArrayGenerator::Int32(num_batch.length, 2)); |
There was a problem hiding this comment.
Why not a scalar here? (and remove #include "arrow/testing/generator.h" above)
| Datum two(ConstantArrayGenerator::Int32(num_batch.length, 2)); | |
| Datum two(Int32Scalar(2)); |
| ARROW_ASSIGN_OR_RAISE(Datum div_two, Divide(num_batch.values[i], two)); | ||
| ARROW_ASSIGN_OR_RAISE(Datum rounded, Multiply(div_two, two)); | ||
| ARROW_ASSIGN_OR_RAISE(Datum low_bit, Subtract(num_batch.values[i], rounded)); | ||
| ARROW_ASSIGN_OR_RAISE(Datum as_bool, Cast(low_bit, type)); |
There was a problem hiding this comment.
Hmm, we have a bit_wise_and kernel but I don't think there is a C++ helper function (e.g. BitWiseAnd) wrapping it. This is probably fine.
|
Benchmark runs are scheduled for baseline = 6697826 and contender = 84c9ac7. 84c9ac7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
See https://issues.apache.org/jira/browse/ARROW-18342