Get rid of header iterator

cpp/src/arrow/flight/CMakeLists.txt

@@ -81,8 +81,6 @@ add_custom_target(flight_grpc_gen ALL DEPENDS ${FLIGHT_GENERATED_PROTO_FILES})
 set(ARROW_FLIGHT_SRCS
     client.cc
     internal.cc
-    middleware.cc
-    middleware_internal.cc
     protocol_internal.cc
     serialization_internal.cc
     server.cc

cpp/src/arrow/flight/client.cc

@@ -125,7 +125,11 @@ class GrpcClientInterceptorAdapter : public grpc::experimental::Interceptor {
     // (grpc-java and grpc-core differ on this point, it seems)
     if (methods->QueryInterceptionHookPoint(
             InterceptionHookPoints::POST_RECV_INITIAL_METADATA)) {
-      internal::GrpcCallHeaders headers(methods->GetRecvInitialMetadata());
+      CallHeaders headers;
+      for (const auto& entry : *methods->GetRecvInitialMetadata()) {
+        headers.insert({util::string_view(entry.first.data(), entry.first.length()),
+                        util::string_view(entry.second.data(), entry.second.length())});
+      }
       for (const auto& middleware : middleware_) {
         middleware->ReceivedHeaders(headers);
       }

cpp/src/arrow/flight/flight_test.cc

@@ -618,8 +618,8 @@ class TracingServerMiddlewareFactory : public ServerMiddlewareFactory {
 
   Status StartCall(const CallInfo& info, const CallHeaders& incoming_headers,
                    std::shared_ptr<ServerMiddleware>* middleware) override {
-    const std::pair<HeaderIterator, HeaderIterator>& iter_pair =
-        incoming_headers.GetHeaders("x-tracing-span-id");
+    const std::pair<CallHeaders::const_iterator, CallHeaders::const_iterator>& iter_pair =
+        incoming_headers.equal_range("x-tracing-span-id");
     if (iter_pair.first != iter_pair.second) {
       const util::string_view& value = (*iter_pair.first).second;
       current_span_id = std::string(value);
@@ -1408,61 +1408,5 @@ TEST_F(TestPropagatingMiddleware, DoPut) {
   ValidateStatus(status, FlightMethod::DoPut);
 }
 
-// Validate the implementation of the call headers wrapper
-TEST_F(TestPropagatingMiddleware, CallHeaders) {
-  std::string header1 = "x-header";
-  std::string header1_value1 = "value";
-  std::string header1_value2 = "value";
-  std::string header2 = "x-header-two";
-  std::string header2_value1 = "value-two";
-
-  internal::GrpcMetadataMap headers;
-  headers.insert({header1, header1_value1});
-  headers.insert({header2, header2_value1});
-  headers.insert({header1, header1_value2});
-
-  internal::GrpcCallHeaders incoming_headers(&headers);
-
-  // Make sure that we didn't implement HeaderIterator::Impl
-  // incorrectly
-  ASSERT_EQ(incoming_headers.cbegin(), incoming_headers.cbegin());
-  ASSERT_EQ(incoming_headers.cend(), incoming_headers.cend());
-  ASSERT_EQ(2, incoming_headers.Count(header1));
-  ASSERT_EQ(1, incoming_headers.Count(header2));
-  {
-    const auto& header_pair = incoming_headers.GetHeaders("x-invalid");
-    ASSERT_EQ(header_pair.first, header_pair.second);
-  }
-  {
-    std::pair<CallHeaders::const_iterator, CallHeaders::const_iterator> header_pair =
-        incoming_headers.GetHeaders(header1);
-    auto& current = header_pair.first;
-    ASSERT_NE(current, header_pair.second);
-    CheckHeader(header1, header1_value1, current);
-    ++current;
-    ASSERT_NE(current, header_pair.second);
-    CheckHeader(header1, header1_value2, current);
-    ++current;
-    ASSERT_EQ(current, header_pair.second);
-  }
-  {
-    std::pair<CallHeaders::const_iterator, CallHeaders::const_iterator> header_pair =
-        incoming_headers.GetHeaders(header2);
-    auto& current = header_pair.first;
-    ASSERT_NE(current, header_pair.second);
-    CheckHeader(header2, header2_value1, current);
-    ++current;
-    ASSERT_EQ(current, header_pair.second);
-  }
-  CallHeaders::const_iterator it = incoming_headers.cbegin();
-  ASSERT_NE(it, incoming_headers.cend());
-  it++;
-  ASSERT_NE(it, incoming_headers.cend());
-  it++;
-  ASSERT_NE(it, incoming_headers.cend());
-  it++;
-  ASSERT_EQ(it, incoming_headers.cend());
-}
-
 }  // namespace flight
 }  // namespace arrow

cpp/src/arrow/flight/middleware.cc

@@ -32,46 +32,5 @@
 #include "arrow/util/stl.h"
 
 namespace arrow {
-namespace flight {
-
-HeaderIterator::HeaderIterator() : impl_(nullptr) {}
-
-HeaderIterator::HeaderIterator(std::unique_ptr<Impl> impl) : impl_(impl.release()) {}
-
-HeaderIterator::HeaderIterator(const HeaderIterator& copy)
-    : HeaderIterator(copy.impl_->Clone()) {}
-
-const HeaderIterator::value_type& HeaderIterator::operator*() const {
-  ARROW_CHECK_NE(impl_, nullptr);
-  return impl_->Dereference();
-}
-
-HeaderIterator& HeaderIterator::operator=(const HeaderIterator& other) {
-  impl_ = other.impl_->Clone();
-  return *this;
-}
-
-HeaderIterator& HeaderIterator::operator++() {
-  ARROW_CHECK_NE(impl_, nullptr);
-  impl_->Next();
-  return *this;
-}
-
-HeaderIterator HeaderIterator::operator++(int) {
-  HeaderIterator ret = *this;
-  this->operator++();
-  return ret;
-}
-
-bool HeaderIterator::operator==(const HeaderIterator& r) const {
-  ARROW_CHECK_NE(impl_, nullptr);
-  return impl_->Equals(r.impl_.get());
-}
-
-bool HeaderIterator::operator!=(const HeaderIterator& r) const {
-  ARROW_CHECK_NE(impl_, nullptr);
-  return !(*this == r);
-}
-
-}  // namespace flight
+namespace flight {}  // namespace flight
 }  // namespace arrow

cpp/src/arrow/flight/middleware.h

@@ -20,9 +20,9 @@
 
 #pragma once
 
-#include <map>
 #include <memory>
 #include <string>
+#include <unordered_map>
 #include <utility>
 
 #include "arrow/flight/visibility.h"  // IWYU pragma: keep
@@ -33,58 +33,7 @@ namespace arrow {
 
 namespace flight {
 
-struct ARROW_FLIGHT_EXPORT HeaderIterator {
- public:
-  typedef std::pair<util::string_view, util::string_view> value_type;
-
-  class Impl {
-   public:
-    virtual void Next() = 0;
-    virtual const value_type& Dereference() const = 0;
-    virtual bool Equals(const void* other) const = 0;
-    virtual std::unique_ptr<Impl> Clone() const = 0;
-    virtual ~Impl() = default;
-  };
-
-  /// \brief Nullary constructor to allow usage in Cython.
-  /// Do NOT use this constructor yourself.
-  HeaderIterator();
-  explicit HeaderIterator(std::unique_ptr<Impl> impl);
-  HeaderIterator(const HeaderIterator& copy);
-
-  ~HeaderIterator() = default;
-
-  const value_type& operator*() const;
-  HeaderIterator& operator++();
-  HeaderIterator operator++(int);
-  bool operator==(const HeaderIterator& r) const;
-  bool operator!=(const HeaderIterator& r) const;
-  HeaderIterator& operator=(const HeaderIterator& other);
-
- private:
-  std::unique_ptr<Impl> impl_;
-};
-
-/// \brief A read-only wrapper around headers for an RPC call.
-class ARROW_FLIGHT_EXPORT CallHeaders {
- public:
-  /// \brief The iterator type. Dereferences to a std::pair of string
-  /// views.
-  typedef HeaderIterator const_iterator;
-
-  virtual ~CallHeaders() = default;
-
-  /// \brief Get all the values for the given header.
-  virtual std::pair<const_iterator, const_iterator> GetHeaders(
-      const std::string& key) const = 0;
-  /// \brief Count the number of values for the header (returns 0 if
-  /// header does not exist).
-  virtual std::size_t Count(const std::string& key) const = 0;
-  /// \brief Const iterator for header-value pairs.
-  virtual const_iterator cbegin() const noexcept = 0;
-  /// \brief Const iterator for header-value pairs.
-  virtual const_iterator cend() const noexcept = 0;
-};
+using CallHeaders = std::unordered_multimap<util::string_view, util::string_view>;
 
 /// \brief A write-only wrapper around headers for an RPC call.
 class ARROW_FLIGHT_EXPORT AddCallHeaders {

cpp/src/arrow/flight/middleware_internal.cc

@@ -30,59 +30,6 @@
 namespace arrow {
 namespace flight {
 
-namespace internal {
-
-class MultimapIterator : public HeaderIterator::Impl {
- public:
-  explicit MultimapIterator(GrpcMetadataMap::const_iterator it) : it_(it) {}
-  ~MultimapIterator() override = default;
-
-  void Next() override { it_++; }
-
-  const HeaderIterator::value_type& Dereference() const override {
-    current_header_ = util::string_view(it_->first.data(), it_->first.length());
-    current_value_ = util::string_view(it_->second.data(), it_->second.length());
-    current_ = std::make_pair(current_header_, current_value_);
-    return current_;
-  }
-
-  bool Equals(const void* other) const override {
-    return it_ == static_cast<const MultimapIterator*>(other)->it_;
-  }
-
-  std::unique_ptr<HeaderIterator::Impl> Clone() const override {
-    return arrow::internal::make_unique<MultimapIterator>(it_);
-  }
-
- private:
-  GrpcMetadataMap::const_iterator it_;
-  mutable util::string_view current_header_;
-  mutable util::string_view current_value_;
-  mutable std::pair<util::string_view, util::string_view> current_;
-};
-
-std::pair<CallHeaders::const_iterator, CallHeaders::const_iterator>
-GrpcCallHeaders::GetHeaders(const std::string& key) const {
-  const auto& result = metadata_->equal_range(key);
-  return std::make_pair(
-      HeaderIterator(arrow::internal::make_unique<MultimapIterator>(result.first)),
-      HeaderIterator(arrow::internal::make_unique<MultimapIterator>(result.second)));
-}
-
-std::size_t GrpcCallHeaders::Count(const std::string& key) const {
-  return metadata_->count(key);
-}
-
-CallHeaders::const_iterator GrpcCallHeaders::cbegin() const noexcept {
-  return HeaderIterator(
-      arrow::internal::make_unique<MultimapIterator>(metadata_->cbegin()));
-}
-
-CallHeaders::const_iterator GrpcCallHeaders::cend() const noexcept {
-  return HeaderIterator(
-      arrow::internal::make_unique<MultimapIterator>(metadata_->cend()));
-}
-
-}  // namespace internal
+namespace internal {}  // namespace internal
 }  // namespace flight
 }  // namespace arrow

cpp/src/arrow/flight/middleware_internal.h

@@ -39,26 +39,7 @@ namespace arrow {
 
 namespace flight {
 
-namespace internal {
-
-typedef std::multimap<grpc::string_ref, grpc::string_ref> GrpcMetadataMap;
-
-class ARROW_FLIGHT_EXPORT GrpcCallHeaders : public CallHeaders {
- public:
-  explicit GrpcCallHeaders(const GrpcMetadataMap* metadata) : metadata_(metadata) {}
-  ~GrpcCallHeaders() override = default;
-
-  std::pair<const_iterator, const_iterator> GetHeaders(
-      const std::string& key) const override;
-  std::size_t Count(const std::string& key) const override;
-  const_iterator cbegin() const noexcept override;
-  const_iterator cend() const noexcept override;
-
- private:
-  const GrpcMetadataMap* metadata_;
-};
-
-}  // namespace internal
+namespace internal {}  // namespace internal
 
 }  // namespace flight
 

cpp/src/arrow/flight/server.cc

@@ -350,7 +350,13 @@ class FlightServiceImpl : public FlightService::Service {
                                GrpcServerCallContext& flight_context) {
     // Run server middleware
     const CallInfo info{method};
-    const internal::GrpcCallHeaders incoming_headers(&context->client_metadata());
+    CallHeaders incoming_headers;
+    for (const auto& entry : context->client_metadata()) {
+      incoming_headers.insert(
+          {util::string_view(entry.first.data(), entry.first.length()),
+           util::string_view(entry.second.data(), entry.second.length())});
+    }
+
     GrpcAddCallHeaders outgoing_headers(context);
     for (const auto& factory : middleware_) {
       std::shared_ptr<ServerMiddleware> instance;

docs/source/cpp/api/flight.rst

@@ -43,7 +43,7 @@ Common Types
    :project: arrow_cpp
    :members:
 
-.. doxygenclass:: arrow::flight::CallInfo
+.. doxygenstruct:: arrow::flight::CallInfo
    :project: arrow_cpp
    :members:
 

python/pyarrow/_flight.pyx

@@ -1692,7 +1692,7 @@ cdef CStatus _middleware_received_headers(
 
 cdef dict convert_headers(const CCallHeaders& c_headers):
     cdef:
-        CHeaderIterator header_iter = c_headers.cbegin()
+        CCallHeaders.const_iterator header_iter = c_headers.cbegin()
     headers = {}
     while header_iter != c_headers.cend():
         header = frombytes(c_string(deref(header_iter).first))

python/pyarrow/includes/libarrow_flight.pxd

@@ -226,16 +226,18 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
     cdef cppclass CCallInfo" arrow::flight::CallInfo":
         CFlightMethod method
 
-    cdef cppclass CHeaderIterator" arrow::flight::HeaderIterator":
-        CHeaderIterator& operator++()
-        const pair[c_string, c_string] operator*()
-        bint operator==(const CHeaderIterator& r)
-        bint operator!=(const CHeaderIterator& r)
-
+    # This is really std::unordered_multimap, but Cython has no
+    # bindings for it, so treat it as an opaque class and bind the
+    # methods we need
     cdef cppclass CCallHeaders" arrow::flight::CallHeaders":
-        size_t Count(const c_string& key)
-        CHeaderIterator cbegin()
-        CHeaderIterator cend()
+        cppclass const_iterator:
+            pair[c_string, c_string] operator*()
+            const_iterator operator++()
+            bint operator==(const_iterator)
+            bint operator!=(const_iterator)
+
+        const_iterator cbegin()
+        const_iterator cend()
 
     cdef cppclass CAddCallHeaders" arrow::flight::AddCallHeaders":
         void AddHeader(const c_string& key, const c_string& value)