Address review

cpp/src/arrow/compute/kernel.h

@@ -64,6 +64,10 @@ struct Datum;
 static inline bool CollectionEquals(const std::vector<Datum>& left,
                                     const std::vector<Datum>& right);
 
+// Datums variants may have a length. This special value indicate that the
+// current variant does not have a length.
+constexpr int64_t kUnknownLength = -1;
+
 /// \class Datum
 /// \brief Variant type for various Arrow C++ data structures
 struct ARROW_EXPORT Datum {
@@ -203,7 +207,7 @@ struct ARROW_EXPORT Datum {
 
   /// \brief The value length of the variant, if any
   ///
-  /// \return -1 if no type
+  /// \return kUnknownLength if no type
   int64_t length() const {
     if (this->kind() == Datum::ARRAY) {
       return util::get<std::shared_ptr<ArrayData>>(this->value)->length;
@@ -212,7 +216,7 @@ struct ARROW_EXPORT Datum {
     } else if (this->kind() == Datum::SCALAR) {
       return 1;
     }
-    return -1;
+    return kUnknownLength;
   }
 
   bool Equals(const Datum& other) const {

cpp/src/arrow/compute/kernels/compare.cc

@@ -35,13 +35,13 @@ template <typename ArrowType, CompareOperator Op,
           typename ScalarType = typename TypeTraits<ArrowType>::ScalarType,
           typename T = typename TypeTraits<ArrowType>::CType>
 static Status CompareArrayScalar(const ArrayData& array, const ScalarType& scalar,
-                                 uint8_t* bitmap) {
+                                 uint8_t* output_bitmap) {
   const T* left = array.GetValues<T>(1);
   const T right = scalar.value;
 
-  internal::GenerateBitsUnrolled(bitmap, 0, array.length, [&left, right]() -> bool {
-    return Comparator<T, Op>::Compare(*left++, right);
-  });
+  internal::GenerateBitsUnrolled(
+      output_bitmap, 0, array.length,
+      [&left, right]() -> bool { return Comparator<T, Op>::Compare(*left++, right); });
 
   return Status::OK();
 }
@@ -50,13 +50,13 @@ template <typename ArrowType, CompareOperator Op,
           typename ScalarType = typename TypeTraits<ArrowType>::ScalarType,
           typename T = typename TypeTraits<ArrowType>::CType>
 static Status CompareScalarArray(const ScalarType& scalar, const ArrayData& array,
-                                 uint8_t* bitmap) {
+                                 uint8_t* output_bitmap) {
   const T left = scalar.value;
   const T* right = array.GetValues<T>(1);
 
-  internal::GenerateBitsUnrolled(bitmap, 0, array.length, [left, &right]() -> bool {
-    return Comparator<T, Op>::Compare(left, *right++);
-  });
+  internal::GenerateBitsUnrolled(
+      output_bitmap, 0, array.length,
+      [left, &right]() -> bool { return Comparator<T, Op>::Compare(left, *right++); });
 
   return Status::OK();
 }
@@ -64,11 +64,11 @@ static Status CompareScalarArray(const ScalarType& scalar, const ArrayData& arra
 template <typename ArrowType, CompareOperator Op,
           typename T = typename TypeTraits<ArrowType>::CType>
 static Status CompareArrayArray(const ArrayData& lhs, const ArrayData& rhs,
-                                uint8_t* bitmap) {
+                                uint8_t* output_bitmap) {
   const T* left = lhs.GetValues<T>(1);
   const T* right = rhs.GetValues<T>(1);
 
-  internal::GenerateBitsUnrolled(bitmap, 0, lhs.length, [&left, &right]() -> bool {
+  internal::GenerateBitsUnrolled(output_bitmap, 0, lhs.length, [&left, &right]() -> bool {
     return Comparator<T, Op>::Compare(*left++, *right++);
   });
 
@@ -135,7 +135,7 @@ class CompareFunction final : public FilterFunction {
     DCHECK_EQ(output->length, lhs.length);
 
     // Copy null_bitmap
-    RETURN_NOT_OK(detail::PropagateNulls(ctx_, lhs, rhs, output));
+    RETURN_NOT_OK(detail::AssignNullIntersection(ctx_, lhs, rhs, output));
 
     uint8_t* bitmap_result = output->buffers[1]->mutable_data();
     return CompareArrayArray<ArrowType, Op>(lhs, rhs, bitmap_result);

cpp/src/arrow/compute/kernels/util-internal.cc

@@ -238,39 +238,7 @@ Status PropagateNulls(FunctionContext* ctx, const ArrayData& input, ArrayData* o
 
 Status PropagateNulls(FunctionContext* ctx, const ArrayData& lhs, const ArrayData& rhs,
                       ArrayData* output) {
-  const int64_t length = lhs.length;
-
-  int64_t lhs_null_count = lhs.GetNullCount();
-  int64_t rhs_null_count = rhs.GetNullCount();
-
-  if (lhs_null_count == 0 && rhs_null_count == 0) {
-    // Both null, nothing to do.
-    output->null_count = 0;
-    return Status::OK();
-  } else if (lhs_null_count != 0 && rhs_null_count == 0) {
-    // Propagate lhs nulls
-    return PropagateNulls(ctx, lhs, output);
-  } else if (lhs_null_count == 0 && rhs_null_count != 0) {
-    // Propagate rhs nulls
-    return PropagateNulls(ctx, rhs, output);
-  }
-
-  if (output->buffers.size() == 0) {
-    // Ensure we can assign a buffer
-    output->buffers.resize(1);
-  }
-
-  std::shared_ptr<Buffer> buffer;
-
-  auto lhs_validity_data = lhs.buffers[0]->data();
-  auto rhs_validity_data = rhs.buffers[0]->data();
-  RETURN_NOT_OK(internal::BitmapAnd(ctx->memory_pool(), lhs_validity_data, lhs.offset,
-                                    rhs_validity_data, rhs.offset, length,
-                                    0 /* out_offset */, &buffer));
-
-  output->null_count = length - internal::CountSetBits(buffer->data(), 0, length);
-  output->buffers[0] = std::move(buffer);
-  return Status::OK();
+  return AssignNullIntersection(ctx, lhs, rhs, output);
 }
 
 Status SetAllNulls(FunctionContext* ctx, const ArrayData& input, ArrayData* output) {