ARROW-1525: [C++] New compare functions that return boolean instead of Status

Comparison should always succeed so, it makes better sense to return boolean
values as the result of comparison instead of Status.
This changeset introduces a set of new compare functions and deprecates the
current ones.

Author: Amir Malekpour <a.malekpour@gmail.com>

cpp/README.md

@@ -248,15 +248,15 @@ to avoid failures due to compiler warnings.
 Note that the clang-tidy target may take a while to run.  You might consider
 running clang-tidy separately on the files you have added/changed before
 invoking the make target to reduce iteration time.  Also, it might generate warnings
-that aren't valid.  To avoid these you can use add a line comment `// NOLINT`. If
+that aren't valid.  To avoid these you can add a line comment `// NOLINT`. If
 NOLINT doesn't suppress the warnings, you add the file in question to
 the .clang-tidy-ignore file.  This will allow `make check-clang-tidy` to pass in
-travis-CI (but still surface the potential warnings in `make clang-tidy`).   Ideally,
-both of these options would be used rarely.  Current known uses-cases whent hey are required:
+travis-CI (but still surface the potential warnings in `make clang-tidy`). Ideally,
+both of these options would be used rarely. Current known uses-cases when they are required:
 
 *  Parameterized tests in google test.
 
 [1]: https://brew.sh/
 [2]: https://github.com/apache/arrow/blob/master/cpp/apidoc/Windows.md
 [3]: https://google.github.io/styleguide/cppguide.html
-[4]: https://github.com/include-what-you-use/include-what-you-use
+[4]: https://github.com/include-what-you-use/include-what-you-use

cpp/src/arrow/array.cc

@@ -53,12 +53,7 @@ int64_t Array::null_count() const {
 }
 
 bool Array::Equals(const Array& arr) const {
-  bool are_equal = false;
-  Status error = ArrayEquals(*this, arr, &are_equal);
-  if (!error.ok()) {
-    DCHECK(false) << "Arrays not comparable: " << error.ToString();
-  }
-  return are_equal;
+  return ArrayEquals(*this, arr);
 }
 
 bool Array::Equals(const std::shared_ptr<Array>& arr) const {
@@ -69,12 +64,7 @@ bool Array::Equals(const std::shared_ptr<Array>& arr) const {
 }
 
 bool Array::ApproxEquals(const Array& arr) const {
-  bool are_equal = false;
-  Status error = ArrayApproxEquals(*this, arr, &are_equal);
-  if (!error.ok()) {
-    DCHECK(false) << "Arrays not comparable: " << error.ToString();
-  }
-  return are_equal;
+  return ArrayApproxEquals(*this, arr);
 }
 
 bool Array::ApproxEquals(const std::shared_ptr<Array>& arr) const {
@@ -94,13 +84,7 @@ bool Array::RangeEquals(int64_t start_idx, int64_t end_idx, int64_t other_start_
 
 bool Array::RangeEquals(const Array& other, int64_t start_idx, int64_t end_idx,
                         int64_t other_start_idx) const {
-  bool are_equal = false;
-  Status error =
-      ArrayRangeEquals(*this, other, start_idx, end_idx, other_start_idx, &are_equal);
-  if (!error.ok()) {
-    DCHECK(false) << "Arrays not comparable: " << error.ToString();
-  }
-  return are_equal;
+  return ArrayRangeEquals(*this, other, start_idx, end_idx, other_start_idx);
 }
 
 static inline std::shared_ptr<ArrayData> SliceData(const ArrayData& data, int64_t offset,

cpp/src/arrow/compare.cc

@@ -542,22 +542,26 @@ static bool BaseDataEquals(const Array& left, const Array& right) {
 }
 
 template <typename VISITOR>
-inline Status ArrayEqualsImpl(const Array& left, const Array& right, bool* are_equal) {
+inline bool ArrayEqualsImpl(const Array& left, const Array& right) {
+  bool are_equal;
   // The arrays are the same object
   if (&left == &right) {
-    *are_equal = true;
+    are_equal = true;
   } else if (!BaseDataEquals(left, right)) {
-    *are_equal = false;
+    are_equal = false;
   } else if (left.length() == 0) {
-    *are_equal = true;
+    are_equal = true;
   } else if (left.null_count() == left.length()) {
-    *are_equal = true;
+    are_equal = true;
   } else {
     VISITOR visitor(right);
-    RETURN_NOT_OK(VisitArrayInline(left, &visitor));
-    *are_equal = visitor.result();
+    auto error = VisitArrayInline(left, &visitor);
+    if (!error.ok()) {
+      DCHECK(false) << "Arrays are not comparable: " << error.ToString();
+    }
+    are_equal = visitor.result();
   }
-  return Status::OK();
+  return are_equal;
 }
 
 class TypeEqualsVisitor {
@@ -668,29 +672,33 @@ class TypeEqualsVisitor {
 
 }  // namespace internal
 
-Status ArrayEquals(const Array& left, const Array& right, bool* are_equal) {
-  return internal::ArrayEqualsImpl<internal::ArrayEqualsVisitor>(left, right, are_equal);
+bool ArrayEquals(const Array& left, const Array& right) {
+  return internal::ArrayEqualsImpl<internal::ArrayEqualsVisitor>(left, right);
 }
 
-Status ArrayApproxEquals(const Array& left, const Array& right, bool* are_equal) {
-  return internal::ArrayEqualsImpl<internal::ApproxEqualsVisitor>(left, right, are_equal);
+bool ArrayApproxEquals(const Array& left, const Array& right) {
+  return internal::ArrayEqualsImpl<internal::ApproxEqualsVisitor>(left, right);
 }
 
-Status ArrayRangeEquals(const Array& left, const Array& right, int64_t left_start_idx,
-                        int64_t left_end_idx, int64_t right_start_idx, bool* are_equal) {
+bool ArrayRangeEquals(const Array& left, const Array& right, int64_t left_start_idx,
+                      int64_t left_end_idx, int64_t right_start_idx) {
+  bool are_equal;
   if (&left == &right) {
-    *are_equal = true;
+    are_equal = true;
   } else if (left.type_id() != right.type_id()) {
-    *are_equal = false;
+    are_equal = false;
   } else if (left.length() == 0) {
-    *are_equal = true;
+    are_equal = true;
   } else {
     internal::RangeEqualsVisitor visitor(right, left_start_idx, left_end_idx,
                                          right_start_idx);
-    RETURN_NOT_OK(VisitArrayInline(left, &visitor));
-    *are_equal = visitor.result();
+    auto error = VisitArrayInline(left, &visitor);
+    if (!error.ok()) {
+      DCHECK(false) << "Arrays are not comparable: " << error.ToString();
+    }
+    are_equal = visitor.result();
   }
-  return Status::OK();
+  return are_equal;
 }
 
 bool StridedTensorContentEquals(int dim_index, int64_t left_offset, int64_t right_offset,
@@ -716,24 +724,25 @@ bool StridedTensorContentEquals(int dim_index, int64_t left_offset, int64_t righ
   return true;
 }
 
-Status TensorEquals(const Tensor& left, const Tensor& right, bool* are_equal) {
+bool TensorEquals(const Tensor& left, const Tensor& right) {
+  bool are_equal;
   // The arrays are the same object
   if (&left == &right) {
-    *are_equal = true;
+    are_equal = true;
   } else if (left.type_id() != right.type_id()) {
-    *are_equal = false;
+    are_equal = false;
   } else if (left.size() == 0) {
-    *are_equal = true;
+    are_equal = true;
   } else {
     if (!left.is_contiguous() || !right.is_contiguous()) {
       const auto& shape = left.shape();
       if (shape != right.shape()) {
-        *are_equal = false;
-        return Status::OK();
+        are_equal = false;
+      } else {
+        const auto& type = static_cast<const FixedWidthType&>(*left.type());
+        are_equal = StridedTensorContentEquals(0, 0, 0,
+                                               type.bit_width() / 8, left, right);
       }
-      const auto& type = static_cast<const FixedWidthType&>(*left.type());
-      *are_equal = StridedTensorContentEquals(0, 0, 0, type.bit_width() / 8, left, right);
-      return Status::OK();
     } else {
       const auto& size_meta = dynamic_cast<const FixedWidthType&>(*left.type());
       const int byte_width = size_meta.bit_width() / CHAR_BIT;
@@ -742,24 +751,55 @@ Status TensorEquals(const Tensor& left, const Tensor& right, bool* are_equal) {
       const uint8_t* left_data = left.data()->data();
       const uint8_t* right_data = right.data()->data();
 
-      *are_equal = memcmp(left_data, right_data,
-                          static_cast<size_t>(byte_width * left.size())) == 0;
+      are_equal = memcmp(left_data, right_data,
+                         static_cast<size_t>(byte_width * left.size())) == 0;
     }
   }
+  return are_equal;
+}
+
+bool TypeEquals(const DataType& left, const DataType& right) {
+  bool are_equal;
+  // The arrays are the same object
+  if (&left == &right) {
+    are_equal = true;
+  } else if (left.id() != right.id()) {
+    are_equal = false;
+  } else {
+    internal::TypeEqualsVisitor visitor(right);
+    auto error = VisitTypeInline(left, &visitor);
+    if (!error.ok()) {
+      DCHECK(false) << "Types are not comparable: " << error.ToString();
+    }
+    are_equal = visitor.result();
+  }
+  return are_equal;
+}
+
+Status ArrayEquals(const Array& left, const Array& right, bool* are_equal) {
+  *are_equal = ArrayEquals(left, right);
+  return Status::OK();
+}
+
+Status TensorEquals(const Tensor& left, const Tensor& right, bool* are_equal) {
+  *are_equal = TensorEquals(left, right);
+  return Status::OK();
+}
+
+Status ArrayApproxEquals(const Array& left, const Array& right, bool* are_equal) {
+  *are_equal = ArrayApproxEquals(left, right);
+  return Status::OK();
+}
+
+Status ArrayRangeEquals(const Array& left, const Array& right,
+                        int64_t start_idx, int64_t end_idx,
+                        int64_t other_start_idx, bool* are_equal) {
+  *are_equal = ArrayRangeEquals(left, right, start_idx, end_idx, other_start_idx);
   return Status::OK();
 }
 
 Status TypeEquals(const DataType& left, const DataType& right, bool* are_equal) {
-  // The arrays are the same object
-  if (&left == &right) {
-    *are_equal = true;
-  } else if (left.id() != right.id()) {
-    *are_equal = false;
-  } else {
-    internal::TypeEqualsVisitor visitor(right);
-    RETURN_NOT_OK(VisitTypeInline(left, &visitor));
-    *are_equal = visitor.result();
-  }
+  *are_equal = TypeEquals(left, right);
   return Status::OK();
 }
 

cpp/src/arrow/compare.h

@@ -31,25 +31,49 @@ class DataType;
 class Status;
 class Tensor;
 
+#ifndef ARROW_NO_DEPRECATED_API
 /// Returns true if the arrays are exactly equal
+/// \deprecated Since 0.8.0
 Status ARROW_EXPORT ArrayEquals(const Array& left, const Array& right, bool* are_equal);
 
+/// \deprecated Since 0.8.0
 Status ARROW_EXPORT TensorEquals(const Tensor& left, const Tensor& right,
                                  bool* are_equal);
 
 /// Returns true if the arrays are approximately equal. For non-floating point
 /// types, this is equivalent to ArrayEquals(left, right)
+/// \deprecated Since 0.8.0
 Status ARROW_EXPORT ArrayApproxEquals(const Array& left, const Array& right,
                                       bool* are_equal);
 
 /// Returns true if indicated equal-length segment of arrays is exactly equal
+/// \deprecated Since 0.8.0
 Status ARROW_EXPORT ArrayRangeEquals(const Array& left, const Array& right,
                                      int64_t start_idx, int64_t end_idx,
                                      int64_t other_start_idx, bool* are_equal);
 
 /// Returns true if the type metadata are exactly equal
+/// \deprecated Since 0.8.0
 Status ARROW_EXPORT TypeEquals(const DataType& left, const DataType& right,
                                bool* are_equal);
+#endif
+
+/// Returns true if the arrays are exactly equal
+bool ARROW_EXPORT ArrayEquals(const Array& left, const Array& right);
+
+bool ARROW_EXPORT TensorEquals(const Tensor& left, const Tensor& right);
+
+/// Returns true if the arrays are approximately equal. For non-floating point
+/// types, this is equivalent to ArrayEquals(left, right)
+bool ARROW_EXPORT ArrayApproxEquals(const Array& left, const Array& right);
+
+/// Returns true if indicated equal-length segment of arrays is exactly equal
+bool ARROW_EXPORT ArrayRangeEquals(const Array& left, const Array& right,
+                                   int64_t start_idx, int64_t end_idx,
+                                   int64_t other_start_idx);
+
+/// Returns true if the type metadata are exactly equal
+bool ARROW_EXPORT TypeEquals(const DataType& left, const DataType& right);
 
 }  // namespace arrow
 

cpp/src/arrow/compute/compute-test.cc

@@ -47,8 +47,7 @@ namespace arrow {
 namespace compute {
 
 void AssertArraysEqual(const Array& left, const Array& right) {
-  bool are_equal = false;
-  ASSERT_OK(ArrayEquals(left, right, &are_equal));
+  bool are_equal = ArrayEquals(left, right);
 
   if (!are_equal) {
     std::stringstream ss;

cpp/src/arrow/tensor.cc

@@ -118,12 +118,7 @@ bool Tensor::is_column_major() const {
 Type::type Tensor::type_id() const { return type_->id(); }
 
 bool Tensor::Equals(const Tensor& other) const {
-  bool are_equal = false;
-  Status error = TensorEquals(*this, other, &are_equal);
-  if (!error.ok()) {
-    DCHECK(false) << "Tensors not comparable: " << error.ToString();
-  }
-  return are_equal;
+  return TensorEquals(*this, other);
 }
 
 }  // namespace arrow

cpp/src/arrow/type.cc

@@ -83,12 +83,7 @@ std::string Field::ToString() const {
 DataType::~DataType() {}
 
 bool DataType::Equals(const DataType& other) const {
-  bool are_equal = false;
-  Status error = TypeEquals(*this, other, &are_equal);
-  if (!error.ok()) {
-    DCHECK(false) << "Types not comparable: " << error.ToString();
-  }
-  return are_equal;
+  return TypeEquals(*this, other);
 }
 
 bool DataType::Equals(const std::shared_ptr<DataType>& other) const {