arrow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From u..@apache.org
Subject arrow git commit: ARROW-1525: [C++] New compare functions that return boolean instead of Status
Date Thu, 05 Oct 2017 08:57:40 GMT
Repository: arrow
Updated Branches:
  refs/heads/master 81319d9c2 -> 8b5b22b15


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>

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

Closes #1156 from amirma/arrow-1525 and squashes the following commits:

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


Project: http://git-wip-us.apache.org/repos/asf/arrow/repo
Commit: http://git-wip-us.apache.org/repos/asf/arrow/commit/8b5b22b1
Tree: http://git-wip-us.apache.org/repos/asf/arrow/tree/8b5b22b1
Diff: http://git-wip-us.apache.org/repos/asf/arrow/diff/8b5b22b1

Branch: refs/heads/master
Commit: 8b5b22b15d2df9c7e7fc52842c1f7b8794c7d40e
Parents: 81319d9
Author: Amir Malekpour <a.malekpour@gmail.com>
Authored: Thu Oct 5 10:57:03 2017 +0200
Committer: Uwe L. Korn <uwelk@xhochy.com>
Committed: Thu Oct 5 10:57:03 2017 +0200

----------------------------------------------------------------------
 cpp/README.md                         |   8 +-
 cpp/src/arrow/array.cc                |  22 +-----
 cpp/src/arrow/compare.cc              | 114 +++++++++++++++++++----------
 cpp/src/arrow/compare.h               |  24 ++++++
 cpp/src/arrow/compute/compute-test.cc |   3 +-
 cpp/src/arrow/tensor.cc               |   7 +-
 cpp/src/arrow/type.cc                 |   7 +-
 7 files changed, 111 insertions(+), 74 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/README.md
----------------------------------------------------------------------
diff --git a/cpp/README.md b/cpp/README.md
index 6e29e6f..a94699b 100644
--- a/cpp/README.md
+++ b/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
\ No newline at end of file
+[4]: https://github.com/include-what-you-use/include-what-you-use

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/array.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/array.cc b/cpp/src/arrow/array.cc
index 12922ae..a3a1e62 100644
--- a/cpp/src/arrow/array.cc
+++ b/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,

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/compare.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/compare.cc b/cpp/src/arrow/compare.cc
index 515b8f6..984537f 100644
--- a/cpp/src/arrow/compare.cc
+++ b/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 Status::OK();
+  return are_equal;
 }
 
-Status TypeEquals(const DataType& left, const DataType& right, bool* 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;
+    are_equal = true;
   } else if (left.id() != right.id()) {
-    *are_equal = false;
+    are_equal = false;
   } else {
     internal::TypeEqualsVisitor visitor(right);
-    RETURN_NOT_OK(VisitTypeInline(left, &visitor));
-    *are_equal = visitor.result();
+    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) {
+  *are_equal = TypeEquals(left, right);
   return Status::OK();
 }
 

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/compare.h
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/compare.h b/cpp/src/arrow/compare.h
index a36b553..27176ed 100644
--- a/cpp/src/arrow/compare.h
+++ b/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
 

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/compute/compute-test.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/compute/compute-test.cc b/cpp/src/arrow/compute/compute-test.cc
index 9df4573..9db0a0e 100644
--- a/cpp/src/arrow/compute/compute-test.cc
+++ b/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;

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/tensor.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/tensor.cc b/cpp/src/arrow/tensor.cc
index efadd7c..ed7da8b 100644
--- a/cpp/src/arrow/tensor.cc
+++ b/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

http://git-wip-us.apache.org/repos/asf/arrow/blob/8b5b22b1/cpp/src/arrow/type.cc
----------------------------------------------------------------------
diff --git a/cpp/src/arrow/type.cc b/cpp/src/arrow/type.cc
index b9e3144..14ddd2a 100644
--- a/cpp/src/arrow/type.cc
+++ b/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 {


Mime
View raw message