ARROW-6847: [C++] Add range_expression adapter to Iterator

bkietz · fsaintjacques · commit e080766e742d · 2019-10-16T13:08:30.000-04:00
Allows `Iterator<T>` to be used in a range-for loop Closes apache#5621 from bkietz/6847-Add-a-range-expression-in and squashes the following commits: 927ae0f <Benjamin Kietzman> document equality comparability of Iterator (necessary for Iterator of Iterators) 5ad05d2 <Benjamin Kietzman> add ToStringOstreamable mixin for automatically defining operator<< def95f1 <Benjamin Kietzman> added EqualityComparable helper for defining equality comparison consistently e020e34 <Benjamin Kietzman> refactor to iterate over Result<> a9537ef <Benjamin Kietzman> add test for failing iterator fda9326 <Benjamin Kietzman> ARROW-6847: Add range_expression adapter to Iterator Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: François Saint-Jacques <fsaintjacques@gmail.com>
diff --git a/cpp/src/arrow/result.h b/cpp/src/arrow/result.h
@@ -22,6 +22,7 @@
 #include <utility>
 
 #include "arrow/status.h"
+#include "arrow/util/compare.h"
 #include "arrow/util/macros.h"
 #include "arrow/util/variant.h"
 
@@ -86,7 +87,7 @@ ARROW_EXPORT void DieWithMessage(const std::string& msg);
 ///   arrow::Result<int> CalculateFoo();
 /// ```
 template <class T>
-class Result {
+class Result : public util::EqualityComparable<Result<T>> {
   template <typename U>
   friend class Result;
   using VariantType = arrow::util::variant<T, Status, const char*>;
@@ -215,6 +216,9 @@ class Result {
     return *this;
   }
 
+  /// Compare to another Result.
+  bool Equals(const Result& other) const { return variant_ == other.variant_; }
+
   /// Indicates whether the object contains a `T` value.  Generally instead
   /// of accessing this directly you will want to use ASSIGN_OR_RAISE defined
   /// below.
diff --git a/cpp/src/arrow/result_test.cc b/cpp/src/arrow/result_test.cc
@@ -585,5 +585,23 @@ TEST(ResultTest, TemplateMoveConstruct) {
   //  NOLINTNEXTLINE use after move.
 }
 
+TEST(ResultTest, Equality) {
+  EXPECT_EQ(Result<int>(), Result<int>());
+  EXPECT_EQ(Result<int>(3), Result<int>(3));
+  EXPECT_EQ(Result<int>(Status::Invalid("error")), Result<int>(Status::Invalid("error")));
+
+  EXPECT_NE(Result<int>(), Result<int>(3));
+  EXPECT_NE(Result<int>(Status::Invalid("error")), Result<int>(3));
+  EXPECT_NE(Result<int>(3333), Result<int>(0));
+  EXPECT_NE(Result<int>(Status::Invalid("error")),
+            Result<int>(Status::Invalid("other error")));
+
+  Result<int> moved_from(3);
+  auto moved_to = std::move(moved_from);
+  //  NOLINTNEXTLINE use after move.
+  EXPECT_NE(Result<int>(3), moved_from);
+  //  NOLINTNEXTLINE use after move.
+}
+
 }  // namespace
 }  // namespace arrow
diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h
@@ -21,6 +21,7 @@
 #include <string>
 #include <utility>
 
+#include "arrow/util/compare.h"
 #include "arrow/util/macros.h"
 #include "arrow/util/string_builder.h"
 #include "arrow/util/visibility.h"
@@ -110,6 +111,10 @@ class ARROW_EXPORT StatusDetail {
   virtual const char* type_id() const = 0;
   /// \brief Produce a human-readable description of this status.
   virtual std::string ToString() const = 0;
+
+  bool operator==(const StatusDetail& other) const noexcept {
+    return std::string(type_id()) == other.type_id() && ToString() == other.ToString();
+  }
 };
 
 /// \brief Status outcome object (success or error)
@@ -120,7 +125,8 @@ class ARROW_EXPORT StatusDetail {
 ///
 /// Additionally, if an error occurred, a specific error message is generally
 /// attached.
-class ARROW_EXPORT Status {
+class ARROW_EXPORT Status : public util::EqualityComparable<Status>,
+                            public util::ToStringOstreamable<Status> {
  public:
   // Create a success status.
   Status() noexcept : state_(NULLPTR) {}
@@ -144,6 +150,8 @@ class ARROW_EXPORT Status {
   inline Status(Status&& s) noexcept;
   inline Status& operator=(Status&& s) noexcept;
 
+  inline bool Equals(const Status& s) const;
+
   // AND the statuses.
   inline Status operator&(const Status& s) const noexcept;
   inline Status operator&(Status&& s) const noexcept;
@@ -153,12 +161,6 @@ class ARROW_EXPORT Status {
   /// Return a success status
   static Status OK() { return Status(); }
 
-  /// Return a success status with a specific message
-  template <typename... Args>
-  static Status OK(Args&&... args) {
-    return Status(StatusCode::OK, util::StringBuilder(std::forward<Args>(args)...));
-  }
-
   /// Return an error status for out-of-memory conditions
   template <typename... Args>
   static Status OutOfMemory(Args&&... args) {
@@ -347,11 +349,6 @@ class ARROW_EXPORT Status {
   inline void MoveFrom(Status& s);
 };
 
-static inline std::ostream& operator<<(std::ostream& os, const Status& x) {
-  os << x.ToString();
-  return os;
-}
-
 void Status::MoveFrom(Status& s) {
   delete state_;
   state_ = s.state_;
@@ -377,6 +374,22 @@ Status& Status::operator=(Status&& s) noexcept {
   return *this;
 }
 
+bool Status::Equals(const Status& s) const {
+  if (state_ == s.state_) {
+    return true;
+  }
+
+  if (ok() || s.ok()) {
+    return false;
+  }
+
+  if (detail() != s.detail() && !(*detail() == *s.detail())) {
+    return false;
+  }
+
+  return code() == s.code() && message() == s.message();
+}
+
 /// \cond FALSE
 // (note: emits warnings on Doxygen < 1.8.15,
 //  see https://github.com/doxygen/doxygen/issues/6295)
diff --git a/cpp/src/arrow/status_test.cc b/cpp/src/arrow/status_test.cc
@@ -106,4 +106,12 @@ TEST(StatusTest, AndStatus) {
   ASSERT_TRUE(res.IsInvalid());
 }
 
+TEST(StatusTest, TestEquality) {
+  ASSERT_EQ(Status(), Status::OK());
+  ASSERT_EQ(Status::Invalid("error"), Status::Invalid("error"));
+
+  ASSERT_NE(Status::Invalid("error"), Status::OK());
+  ASSERT_NE(Status::Invalid("error"), Status::Invalid("other error"));
+}
+
 }  // namespace arrow
diff --git a/cpp/src/arrow/util/compare.h b/cpp/src/arrow/util/compare.h
@@ -0,0 +1,57 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+#include "arrow/util/macros.h"
+
+namespace arrow {
+namespace util {
+
+/// CRTP helper for declaring equality comparison. Defines operator== and operator!=
+template <typename T>
+class EqualityComparable {
+ public:
+  ~EqualityComparable() {
+    static_assert(
+        std::is_same<decltype(std::declval<const T>().Equals(std::declval<const T>())),
+                     bool>::value,
+        "EqualityComparable depends on the method T::Equals(const T&) const");
+  }
+
+  template <typename... Extra>
+  bool Equals(const std::shared_ptr<T>& other, Extra&&... extra) const {
+    if (other == NULLPTR) {
+      return false;
+    }
+    return cast().Equals(*other, std::forward<Extra>(extra)...);
+  }
+
+  bool operator==(const T& other) const { return cast().Equals(other); }
+
+  bool operator!=(const T& other) const { return !(cast() == other); }
+
+ private:
+  const T& cast() const { return static_cast<const T&>(*this); }
+};
+
+}  // namespace util
+}  // namespace arrow
diff --git a/cpp/src/arrow/util/iterator.h b/cpp/src/arrow/util/iterator.h
@@ -24,7 +24,9 @@
 #include <utility>
 #include <vector>
 
+#include "arrow/result.h"
 #include "arrow/status.h"
+#include "arrow/util/compare.h"
 #include "arrow/util/functional.h"
 #include "arrow/util/macros.h"
 #include "arrow/util/visibility.h"
@@ -44,7 +46,7 @@ struct IterationTraits {
 
 /// \brief A generic Iterator that can return errors
 template <typename T>
-class Iterator {
+class Iterator : public util::EqualityComparable<Iterator<T>> {
  public:
   /// \brief Iterator may be constructed from any type which has a member function
   /// with signature Status Next(T*);
@@ -87,11 +89,54 @@ class Iterator {
     return status;
   }
 
-  /// Iterators will only compare equal if they are both null
-  bool operator==(const Iterator& other) const { return ptr_ == other.ptr_; }
+  /// Iterators will only compare equal if they are both null.
+  /// Equality comparability is required to make an Iterator of Iterators
+  /// (to check for the end condition).
+  bool Equals(const Iterator& other) const { return ptr_ == other.ptr_; }
 
   explicit operator bool() const { return ptr_ != NULLPTR; }
 
+  class RangeIterator {
+   public:
+    RangeIterator() = default;
+
+    explicit RangeIterator(Iterator i) : iterator_(std::move(i)) { Next(); }
+
+    bool operator!=(const RangeIterator& other) const {
+      return !(status_ == other.status_ && value_ == other.value_);
+    }
+
+    RangeIterator& operator++() {
+      Next();
+      return *this;
+    }
+
+    Result<T> operator*() {
+      if (status_.ok()) {
+        return std::move(value_);
+      }
+      return status_;
+    }
+
+   private:
+    void Next() {
+      if (!status_.ok()) {
+        status_ = Status::OK();
+        value_ = IterationTraits<T>::End();
+        return;
+      }
+      status_ = iterator_.Next(&value_);
+    }
+
+    T value_ = IterationTraits<T>::End();
+    Iterator iterator_;
+    Status status_;
+  };
+
+  RangeIterator begin() { return RangeIterator(std::move(*this)); }
+
+  RangeIterator end() { return RangeIterator(); }
+
  private:
   /// Implementation of deleter for ptr_: Casts from void* to the wrapped type and
   /// deletes that.
diff --git a/cpp/src/arrow/util/iterator_test.cc b/cpp/src/arrow/util/iterator_test.cc
@@ -177,6 +177,48 @@ TEST(TestVectorIterator, Basic) {
   AssertIteratorNoMatch({1, 2, 3, 1}, VectorIt({1, 2, 3}));
 }
 
+TEST(TestVectorIterator, RangeForLoop) {
+  std::vector<TestInt> ints = {1, 2, 3, 4};
+
+  auto ints_it = ints.begin();
+  for (auto v_result : VectorIt(ints)) {
+    ASSERT_OK_AND_ASSIGN(TestInt v, v_result);
+    ASSERT_EQ(v, *ints_it++);
+  }
+  ASSERT_EQ(ints_it, ints.end());
+
+  std::vector<std::unique_ptr<TestInt>> vec;
+  for (TestInt i : ints) {
+    vec.emplace_back(new TestInt(i));
+  }
+
+  // also works with move only types
+  ints_it = ints.begin();
+  for (auto v_result : MakeVectorIterator(std::move(vec))) {
+    ASSERT_OK_AND_ASSIGN(std::unique_ptr<TestInt> v, std::move(v_result));
+    ASSERT_EQ(*v, *ints_it++);
+  }
+  ASSERT_EQ(ints_it, ints.end());
+}
+
+TEST(TestFunctionIterator, RangeForLoop) {
+  int i = 0;
+  auto fails_at_3 = MakeFunctionIterator([&](TestInt* out) {
+    if (i >= 3) {
+      return Status::IndexError("fails at 3");
+    }
+    out->value = i++;
+    return Status::OK();
+  });
+
+  std::vector<Result<TestInt>> actual,
+      expected{0, 1, 2, Status::IndexError("fails at 3")};
+  for (auto r : fails_at_3) {
+    actual.push_back(std::move(r));
+  }
+  ASSERT_EQ(actual, expected);
+}
+
 TEST(FilterIterator, Basic) {
   AssertIteratorMatch({1, 2, 3, 4}, FilterIt(VectorIt({1, 2, 3, 4}),
                                              [](TestInt i, TestInt* o, bool* accept) {
diff --git a/cpp/src/arrow/util/string_builder.h b/cpp/src/arrow/util/string_builder.h
@@ -63,6 +63,24 @@ std::string StringBuilder(Args&&... args) {
   return ss.str();
 }
 
+/// CRTP helper for declaring string representation. Defines operator<<
+template <typename T>
+class ToStringOstreamable {
+ public:
+  ~ToStringOstreamable() {
+    static_assert(
+        std::is_same<decltype(std::declval<const T>().ToString()), std::string>::value,
+        "ToStringOstreamable depends on the method T::ToString() const");
+  }
+
+ private:
+  const T& cast() const { return static_cast<const T&>(*this); }
+
+  friend inline std::ostream& operator<<(std::ostream& os, const ToStringOstreamable& t) {
+    return os << t.cast().ToString();
+  }
+};
+
 }  // namespace util
 }  // namespace arrow
 
