ARROW-8727: [C++] Don't require stack allocation of any object to use StringConverter, hide behind ParseValue function

wesm · bkietz · commit 3752e34adf29 · 2020-05-08T16:24:28.000-04:00
Using non-stateful functions or functors is more natural for template-based code generation of e.g. kernels. Closes apache#7120 from wesm/string-converter-static Authored-by: Wes McKinney <wesm+git@apache.org> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
diff --git a/cpp/src/arrow/c/bridge.cc b/cpp/src/arrow/c/bridge.cc
@@ -652,9 +652,8 @@ class FormatStringParser {
   template <typename IntType = int32_t>
   Result<IntType> ParseInt(util::string_view v) {
     using ArrowIntType = typename CTypeTraits<IntType>::ArrowType;
-    internal::StringConverter<ArrowIntType> converter;
     IntType value;
-    if (!converter(v.data(), v.size(), &value)) {
+    if (!internal::ParseValue<ArrowIntType>(v.data(), v.size(), &value)) {
       return Invalid();
     }
     return value;
diff --git a/cpp/src/arrow/compute/kernels/cast.cc b/cpp/src/arrow/compute/kernels/cast.cc
@@ -1073,15 +1073,13 @@ struct CastFunctor<
 
     typename TypeTraits<I>::ArrayType input_array(input.Copy());
     auto out_data = output->GetMutableValues<out_type>(1);
-    internal::StringConverter<O> converter;
-
     for (int64_t i = 0; i < input.length; ++i, ++out_data) {
       if (input_array.IsNull(i)) {
         continue;
       }
 
       auto str = input_array.GetView(i);
-      if (!converter(str.data(), str.length(), out_data)) {
+      if (!internal::ParseValue<O>(str.data(), str.length(), out_data)) {
         ctx->SetStatus(Status::Invalid("Failed to cast String '", str, "' into ",
                                        output->type->ToString()));
         return;
@@ -1100,7 +1098,6 @@ struct CastFunctor<BooleanType, I, enable_if_t<is_string_like_type<I>::value>> {
     typename TypeTraits<I>::ArrayType input_array(input.Copy());
     internal::FirstTimeBitmapWriter writer(output->buffers[1]->mutable_data(),
                                            output->offset, input.length);
-    internal::StringConverter<BooleanType> converter;
 
     for (int64_t i = 0; i < input.length; ++i) {
       if (input_array.IsNull(i)) {
@@ -1110,7 +1107,7 @@ struct CastFunctor<BooleanType, I, enable_if_t<is_string_like_type<I>::value>> {
 
       bool value;
       auto str = input_array.GetView(i);
-      if (!converter(str.data(), str.length(), &value)) {
+      if (!internal::ParseValue<BooleanType>(str.data(), str.length(), &value)) {
         ctx->SetStatus(Status::Invalid("Failed to cast String '",
                                        input_array.GetString(i), "' into ",
                                        output->type->ToString()));
@@ -1139,15 +1136,15 @@ struct CastFunctor<TimestampType, I, enable_if_t<is_string_like_type<I>::value>>
 
     typename TypeTraits<I>::ArrayType input_array(input.Copy());
     auto out_data = output->GetMutableValues<out_type>(1);
-    internal::StringConverter<TimestampType> converter(output->type);
+
+    const TimeUnit::type unit = checked_cast<const TimestampType&>(*output->type).unit();
 
     for (int64_t i = 0; i < input.length; ++i, ++out_data) {
       if (input_array.IsNull(i)) {
         continue;
       }
-
       const auto str = input_array.GetView(i);
-      if (!converter(str.data(), str.length(), out_data)) {
+      if (!internal::ParseTimestampISO8601(str.data(), str.length(), unit, out_data)) {
         ctx->SetStatus(Status::Invalid("Failed to cast String '", str, "' into ",
                                        output->type->ToString()));
         return;
diff --git a/cpp/src/arrow/csv/converter.cc b/cpp/src/arrow/csv/converter.cc
@@ -40,7 +40,6 @@ namespace arrow {
 namespace csv {
 
 using internal::checked_cast;
-using internal::StringConverter;
 using internal::Trie;
 using internal::TrieBuilder;
 
@@ -349,10 +348,9 @@ class NumericConverter : public ConcreteConverter {
   Result<std::shared_ptr<Array>> Convert(const BlockParser& parser,
                                          int32_t col_index) override {
     using BuilderType = typename TypeTraits<T>::BuilderType;
-    using value_type = typename StringConverter<T>::value_type;
+    using value_type = typename T::c_type;
 
     BuilderType builder(type_, pool_);
-    StringConverter<T> converter;
 
     auto visit = [&](const uint8_t* data, uint32_t size, bool quoted) -> Status {
       // XXX should quoted values be allowed at all?
@@ -364,8 +362,8 @@ class NumericConverter : public ConcreteConverter {
       if (!std::is_same<BooleanType, T>::value) {
         TrimWhiteSpace(&data, &size);
       }
-      if (ARROW_PREDICT_FALSE(
-              !converter(reinterpret_cast<const char*>(data), size, &value))) {
+      if (ARROW_PREDICT_FALSE(!internal::ParseValue<T>(
+              reinterpret_cast<const char*>(data), size, &value))) {
         return GenericConversionError(type_, data, size);
       }
       builder.UnsafeAppend(value);
diff --git a/cpp/src/arrow/filesystem/hdfs.cc b/cpp/src/arrow/filesystem/hdfs.cc
@@ -31,6 +31,7 @@
 
 namespace arrow {
 
+using internal::ParseValue;
 using internal::Uri;
 
 namespace fs {
@@ -312,9 +313,8 @@ Result<HdfsOptions> HdfsOptions::FromUri(const Uri& uri) {
   auto it = options_map.find("replication");
   if (it != options_map.end()) {
     const auto& v = it->second;
-    ::arrow::internal::StringConverter<Int16Type> converter;
     int16_t replication;
-    if (!converter(v.data(), v.size(), &replication)) {
+    if (!ParseValue<Int16Type>(v.data(), v.size(), &replication)) {
       return Status::Invalid("Invalid value for option 'replication': '", v, "'");
     }
     options.ConfigureReplication(replication);
@@ -324,9 +324,8 @@ Result<HdfsOptions> HdfsOptions::FromUri(const Uri& uri) {
   it = options_map.find("buffer_size");
   if (it != options_map.end()) {
     const auto& v = it->second;
-    ::arrow::internal::StringConverter<Int32Type> converter;
     int32_t buffer_size;
-    if (!converter(v.data(), v.size(), &buffer_size)) {
+    if (!ParseValue<Int32Type>(v.data(), v.size(), &buffer_size)) {
       return Status::Invalid("Invalid value for option 'buffer_size': '", v, "'");
     }
     options.ConfigureBufferSize(buffer_size);
@@ -336,9 +335,8 @@ Result<HdfsOptions> HdfsOptions::FromUri(const Uri& uri) {
   it = options_map.find("default_block_size");
   if (it != options_map.end()) {
     const auto& v = it->second;
-    ::arrow::internal::StringConverter<Int64Type> converter;
     int64_t default_block_size;
-    if (!converter(v.data(), v.size(), &default_block_size)) {
+    if (!ParseValue<Int64Type>(v.data(), v.size(), &default_block_size)) {
       return Status::Invalid("Invalid value for option 'default_block_size': '", v, "'");
     }
     options.ConfigureBlockSize(default_block_size);
diff --git a/cpp/src/arrow/ipc/json_internal.cc b/cpp/src/arrow/ipc/json_internal.cc
@@ -58,6 +58,7 @@ constexpr char kYearMonth[] = "YEAR_MONTH";
 class MemoryPool;
 
 using internal::checked_cast;
+using internal::ParseValue;
 
 namespace ipc {
 namespace internal {
@@ -1351,6 +1352,7 @@ class ArrayReader {
   template <typename T>
   Status GetIntArray(const RjArray& json_array, const int32_t length,
                      std::shared_ptr<Buffer>* out) {
+    using ArrowType = typename CTypeTraits<T>::ArrowType;
     ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBuffer(length * sizeof(T), pool_));
 
     T* values = reinterpret_cast<T*>(buffer->mutable_data());
@@ -1367,11 +1369,11 @@ class ArrayReader {
     } else {
       // Read 64-bit integers as strings, as JSON numbers cannot represent
       // them exactly.
-      ::arrow::internal::StringConverter<typename CTypeTraits<T>::ArrowType> converter;
+
       for (int i = 0; i < length; ++i) {
         const rj::Value& val = json_array[i];
         DCHECK(val.IsString());
-        if (!converter(val.GetString(), val.GetStringLength(), &values[i])) {
+        if (!ParseValue<ArrowType>(val.GetString(), val.GetStringLength(), &values[i])) {
           return Status::Invalid("Failed to parse integer: '",
                                  std::string(val.GetString(), val.GetStringLength()),
                                  "'");
diff --git a/cpp/src/arrow/ipc/json_simple.cc b/cpp/src/arrow/ipc/json_simple.cc
@@ -39,6 +39,9 @@
 namespace rj = arrow::rapidjson;
 
 namespace arrow {
+
+using internal::ParseValue;
+
 namespace ipc {
 namespace internal {
 namespace json {
@@ -319,7 +322,7 @@ class DecimalConverter final : public ConcreteConverter<DecimalConverter> {
 class TimestampConverter final : public ConcreteConverter<TimestampConverter> {
  public:
   explicit TimestampConverter(const std::shared_ptr<DataType>& type)
-      : from_string_(type) {
+      : parse_ctx_{checked_cast<const TimestampType&>(*type).unit()} {
     this->type_ = type;
     builder_ = std::make_shared<TimestampBuilder>(type, default_memory_pool());
   }
@@ -335,7 +338,7 @@ class TimestampConverter final : public ConcreteConverter<TimestampConverter> {
       RETURN_NOT_OK(ConvertNumber<Int64Type>(json_obj, *this->type_, &value));
     } else if (json_obj.IsString()) {
       auto view = util::string_view(json_obj.GetString(), json_obj.GetStringLength());
-      if (!from_string_(view.data(), view.size(), &value)) {
+      if (!ParseValue<TimestampType>(view.data(), view.size(), &value, &parse_ctx_)) {
         return Status::Invalid("couldn't parse timestamp from ", view);
       }
     } else {
@@ -347,7 +350,7 @@ class TimestampConverter final : public ConcreteConverter<TimestampConverter> {
   std::shared_ptr<ArrayBuilder> builder() override { return builder_; }
 
  private:
-  ::arrow::internal::StringConverter<TimestampType> from_string_;
+  ::arrow::internal::ParseTimestampContext parse_ctx_;
   std::shared_ptr<TimestampBuilder> builder_;
 };
 
diff --git a/cpp/src/arrow/json/converter.cc b/cpp/src/arrow/json/converter.cc
@@ -103,12 +103,38 @@ class BooleanConverter : public PrimitiveConverter {
 };
 
 template <typename T>
-class NumericConverter : public PrimitiveConverter {
+struct ParserAdapter {
+  using value_type = typename T::c_type;
+
+  void InitParser(const DataType& type) {}
+
+  bool ConvertOne(const char* s, size_t length, value_type* out) {
+    return internal::ParseValue<T>(s, length, out);
+  }
+};
+
+template <>
+struct ParserAdapter<TimestampType> {
+  void InitParser(const DataType& type) {
+    this->unit = internal::checked_cast<const TimestampType&>(type).unit();
+  }
+
+  bool ConvertOne(const char* s, size_t length, int64_t* out) {
+    return internal::ParseTimestampISO8601(s, length, unit, out);
+  }
+
+  TimeUnit::type unit;
+};
+
+template <typename T>
+class NumericConverter : public PrimitiveConverter, public ParserAdapter<T> {
  public:
-  using value_type = typename internal::StringConverter<T>::value_type;
+  using value_type = typename T::c_type;
 
   NumericConverter(MemoryPool* pool, const std::shared_ptr<DataType>& type)
-      : PrimitiveConverter(pool, type), convert_one_(type) {}
+      : PrimitiveConverter(pool, type) {
+    ParserAdapter<T>::InitParser(*type);
+  }
 
   Status Convert(const std::shared_ptr<Array>& in, std::shared_ptr<Array>* out) override {
     if (in->type_id() == Type::NA) {
@@ -122,7 +148,7 @@ class NumericConverter : public PrimitiveConverter {
 
     auto visit_valid = [&](string_view repr) {
       value_type value;
-      if (!convert_one_(repr.data(), repr.size(), &value)) {
+      if (!ParserAdapter<T>::ConvertOne(repr.data(), repr.size(), &value)) {
         return GenericConversionError(*out_type_, ", couldn't parse:", repr);
       }
 
@@ -138,8 +164,6 @@ class NumericConverter : public PrimitiveConverter {
     RETURN_NOT_OK(VisitDictionaryEntries(dict_array, visit_valid, visit_null));
     return builder.Finish(out);
   }
-
-  internal::StringConverter<T> convert_one_;
 };
 
 template <typename DateTimeType>
diff --git a/cpp/src/arrow/python/deserialize.cc b/cpp/src/arrow/python/deserialize.cc
@@ -50,7 +50,7 @@
 namespace arrow {
 
 using internal::checked_cast;
-using internal::StringConverter;
+using internal::ParseValue;
 
 namespace py {
 
@@ -232,10 +232,9 @@ Status GetPythonTypes(const UnionArray& data, std::vector<int8_t>* result) {
   ARROW_CHECK(result != nullptr);
   auto type = data.type();
   for (int i = 0; i < type->num_children(); ++i) {
-    StringConverter<Int8Type> converter;
     int8_t tag = 0;
     const std::string& data = type->child(i)->name();
-    if (!converter(data.c_str(), data.size(), &tag)) {
+    if (!ParseValue<Int8Type>(data.c_str(), data.size(), &tag)) {
       return Status::SerializationError("Cannot convert string: \"",
                                         type->child(i)->name(), "\" to int8_t");
     }
diff --git a/cpp/src/arrow/scalar.cc b/cpp/src/arrow/scalar.cc
@@ -220,16 +220,25 @@ std::string Scalar::ToString() const {
 }
 
 struct ScalarParseImpl {
-  template <typename T, typename Converter = internal::StringConverter<T>,
-            typename Value = typename Converter::value_type>
+  // XXX Use of detail here not ideal
+  template <typename T,
+            typename Value = typename internal::detail::StringConverter<T>::value_type>
   Status Visit(const T& t) {
     Value value;
-    if (!Converter{type_}(s_.data(), s_.size(), &value)) {
+    if (!internal::ParseValue<T>(s_.data(), s_.size(), &value)) {
       return Status::Invalid("error parsing '", s_, "' as scalar of type ", t);
     }
     return Finish(std::move(value));
   }
 
+  Status Visit(const TimestampType& t) {
+    int64_t value;
+    if (!internal::ParseTimestampISO8601(s_.data(), s_.size(), t.unit(), &value)) {
+      return Status::Invalid("error parsing '", s_, "' as scalar of type ", t);
+    }
+    return Finish(value);
+  }
+
   Status Visit(const BinaryType&) { return FinishWithBuffer(); }
 
   Status Visit(const LargeBinaryType&) { return FinishWithBuffer(); }
diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc
@@ -180,12 +180,11 @@ static constexpr int64_t kPowersOfTen[kInt64DecimalDigits + 1] = {1LL,
 // the appropriate power of 10 necessary to add source parsed as uint64 and
 // then adds the parsed value of source.
 static inline void ShiftAndAdd(const char* data, size_t length, Decimal128* out) {
-  internal::StringConverter<Int64Type> converter;
   for (size_t posn = 0; posn < length;) {
     const size_t group_size = std::min(kInt64DecimalDigits, length - posn);
     const int64_t multiple = kPowersOfTen[group_size];
     int64_t chunk = 0;
-    ARROW_CHECK(converter(data + posn, group_size, &chunk));
+    ARROW_CHECK(internal::ParseValue<Int64Type>(data + posn, group_size, &chunk));
 
     *out *= multiple;
     *out += chunk;
@@ -275,8 +274,7 @@ bool ParseDecimalComponents(const char* s, size_t size, DecimalComponents* out)
       ++pos;
     }
     out->has_exponent = true;
-    internal::StringConverter<Int32Type> exponent_converter;
-    return exponent_converter(s + pos, size - pos, &(out->exponent));
+    return internal::ParseValue<Int32Type>(s + pos, size - pos, &(out->exponent));
   }
   return pos == size;
 }
diff --git a/cpp/src/arrow/util/uri.cc b/cpp/src/arrow/util/uri.cc
@@ -255,9 +255,8 @@ Status Uri::Parse(const std::string& uri_string) {
   // Parse port number
   auto port_text = TextRangeToView(impl_->uri_.portText);
   if (port_text.size()) {
-    StringConverter<UInt16Type> port_converter;
     uint16_t port_num;
-    if (!port_converter(port_text.data(), port_text.size(), &port_num)) {
+    if (!ParseValue<UInt16Type>(port_text.data(), port_text.size(), &port_num)) {
       return Status::Invalid("Invalid port number '", port_text, "' in URI '", uri_string,
                              "'");
     }
diff --git a/cpp/src/arrow/util/value_parsing.cc b/cpp/src/arrow/util/value_parsing.cc
diff --git a/cpp/src/arrow/util/value_parsing.h b/cpp/src/arrow/util/value_parsing.h
diff --git a/cpp/src/arrow/util/value_parsing_benchmark.cc b/cpp/src/arrow/util/value_parsing_benchmark.cc
diff --git a/cpp/src/arrow/util/value_parsing_test.cc b/cpp/src/arrow/util/value_parsing_test.cc
