[GitHub] [arrow] wjones127 commented on a diff in pull request #14658: ARROW-18342: [C++] AsofJoinNode support for Boolean data field

[GitBox <gi...@apache.org>]

wjones127 commented on code in PR #14658:
URL: https://github.com/apache/arrow/pull/14658#discussion_r1024345220


##########
cpp/src/arrow/compute/exec/asof_join_node_test.cc:
##########
@@ -83,6 +85,7 @@ Result<BatchesWithSchema> MakeBatchesFromNumString(
   batches.schema = schema;
   int n_fields = schema->num_fields();
   for (auto num_batch : num_batches.batches) {
+    Datum two(ConstantArrayGenerator::Int32(num_batch.length, 2));

Review Comment:
   Why not a scalar here? (and remove `#include "arrow/testing/generator.h"` above)
   ```suggestion
       Datum two(Int32Scalar(2));
   ```



##########
cpp/src/arrow/compute/exec/asof_join_node.cc:
##########
@@ -664,12 +666,25 @@ class CompositeReferenceTable {
   }
 
   template <class Type, class Builder = typename TypeTraits<Type>::BuilderType>
-  enable_if_fixed_width_type<Type, Status> static BuilderAppend(
+  enable_if_boolean<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();
     }
+    builder.UnsafeAppend(bit_util::GetBit(source->template GetValues<uint8_t>(1), row));
+    return Status::OK();
+  }
+
+  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();
+    }

Review Comment:
   One pattern we've been moving to in the code base is using constexpr conditions:
   
   ```suggestion
     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.)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org