ARROW-686: [C++] Account for time metadata changes, add Time32 and Time64 types

c_glib/arrow-glib/type.cpp

@@ -72,8 +72,10 @@ garrow_type_from_raw(arrow::Type::type type)
     return GARROW_TYPE_DATE64;
   case arrow::Type::type::TIMESTAMP:
     return GARROW_TYPE_TIMESTAMP;
-  case arrow::Type::type::TIME:
-    return GARROW_TYPE_TIME;
+  case arrow::Type::type::TIME32:
+    return GARROW_TYPE_TIME32;
+  case arrow::Type::type::TIME64:
+    return GARROW_TYPE_TIME64;
   case arrow::Type::type::INTERVAL:
     return GARROW_TYPE_INTERVAL;
   case arrow::Type::type::DECIMAL:

c_glib/arrow-glib/type.h

@@ -44,7 +44,8 @@ G_BEGIN_DECLS
  * @GARROW_TYPE_DATE64: int64 milliseconds since the UNIX epoch.
  * @GARROW_TYPE_TIMESTAMP: Exact timestamp encoded with int64 since UNIX epoch.
  *   Default unit millisecond.
- * @GARROW_TYPE_TIME: Exact time encoded with int64, default unit millisecond.
+ * @GARROW_TYPE_TIME32: Exact time encoded with int32, supporting seconds or milliseconds
+ * @GARROW_TYPE_TIME64: Exact time encoded with int64, supporting micro- or nanoseconds
  * @GARROW_TYPE_INTERVAL: YEAR_MONTH or DAY_TIME interval in SQL style.
  * @GARROW_TYPE_DECIMAL: Precision- and scale-based decimal
  *   type. Storage type depends on the parameters.
@@ -74,7 +75,8 @@ typedef enum {
   GARROW_TYPE_DATE32,
   GARROW_TYPE_DATE64,
   GARROW_TYPE_TIMESTAMP,
-  GARROW_TYPE_TIME,
+  GARROW_TYPE_TIME32,
+  GARROW_TYPE_TIME64,
   GARROW_TYPE_INTERVAL,
   GARROW_TYPE_DECIMAL,
   GARROW_TYPE_LIST,

cpp/src/arrow/array-decimal-test.cc

@@ -29,12 +29,6 @@ TEST(TypesTest, TestDecimalType) {
   ASSERT_EQ(t1.scale, 4);
 
   ASSERT_EQ(t1.ToString(), std::string("decimal(8, 4)"));
-
-  // Test copy constructor
-  DecimalType t2 = t1;
-  ASSERT_EQ(t2.type, Type::DECIMAL);
-  ASSERT_EQ(t2.precision, 8);
-  ASSERT_EQ(t2.scale, 4);
 }
 
 }  // namespace arrow

cpp/src/arrow/array-primitive-test.cc

@@ -47,9 +47,6 @@ class Array;
                                             \
     ASSERT_EQ(tp.type, Type::ENUM);         \
     ASSERT_EQ(tp.ToString(), string(NAME)); \
-                                            \
-    KLASS tp_copy = tp;                     \
-    ASSERT_EQ(tp_copy.type, Type::ENUM);    \
   }
 
 PRIMITIVE_TEST(Int8Type, INT8, "int8");

cpp/src/arrow/array.cc

@@ -483,7 +483,8 @@ template class NumericArray<Int64Type>;
 template class NumericArray<TimestampType>;
 template class NumericArray<Date32Type>;
 template class NumericArray<Date64Type>;
-template class NumericArray<TimeType>;
+template class NumericArray<Time32Type>;
+template class NumericArray<Time64Type>;
 template class NumericArray<HalfFloatType>;
 template class NumericArray<FloatType>;
 template class NumericArray<DoubleType>;

cpp/src/arrow/array.h

@@ -527,10 +527,11 @@ extern template class ARROW_EXPORT NumericArray<UInt64Type>;
 extern template class ARROW_EXPORT NumericArray<HalfFloatType>;
 extern template class ARROW_EXPORT NumericArray<FloatType>;
 extern template class ARROW_EXPORT NumericArray<DoubleType>;
-extern template class ARROW_EXPORT NumericArray<TimestampType>;
 extern template class ARROW_EXPORT NumericArray<Date32Type>;
 extern template class ARROW_EXPORT NumericArray<Date64Type>;
-extern template class ARROW_EXPORT NumericArray<TimeType>;
+extern template class ARROW_EXPORT NumericArray<Time32Type>;
+extern template class ARROW_EXPORT NumericArray<Time64Type>;
+extern template class ARROW_EXPORT NumericArray<TimestampType>;
 
 #if defined(__GNUC__) && !defined(__clang__)
 #pragma GCC diagnostic pop

cpp/src/arrow/builder.cc

@@ -240,8 +240,9 @@ template class PrimitiveBuilder<Int32Type>;
 template class PrimitiveBuilder<Int64Type>;
 template class PrimitiveBuilder<Date32Type>;
 template class PrimitiveBuilder<Date64Type>;
+template class PrimitiveBuilder<Time32Type>;
+template class PrimitiveBuilder<Time64Type>;
 template class PrimitiveBuilder<TimestampType>;
-template class PrimitiveBuilder<TimeType>;
 template class PrimitiveBuilder<HalfFloatType>;
 template class PrimitiveBuilder<FloatType>;
 template class PrimitiveBuilder<DoubleType>;
@@ -511,9 +512,9 @@ std::shared_ptr<ArrayBuilder> StructBuilder::field_builder(int pos) const {
 // ----------------------------------------------------------------------
 // Helper functions
 
-#define BUILDER_CASE(ENUM, BuilderType) \
-  case Type::ENUM:                      \
-    out->reset(new BuilderType(pool));  \
+#define BUILDER_CASE(ENUM, BuilderType)      \
+  case Type::ENUM:                           \
+    out->reset(new BuilderType(pool, type)); \
     return Status::OK();
 
 // Initially looked at doing this with vtables, but shared pointers makes it
@@ -533,17 +534,14 @@ Status MakeBuilder(MemoryPool* pool, const std::shared_ptr<DataType>& type,
     BUILDER_CASE(INT64, Int64Builder);
     BUILDER_CASE(DATE32, Date32Builder);
     BUILDER_CASE(DATE64, Date64Builder);
-    case Type::TIMESTAMP:
-      out->reset(new TimestampBuilder(pool, type));
-      return Status::OK();
-    case Type::TIME:
-      out->reset(new TimeBuilder(pool, type));
-      return Status::OK();
-      BUILDER_CASE(BOOL, BooleanBuilder);
-      BUILDER_CASE(FLOAT, FloatBuilder);
-      BUILDER_CASE(DOUBLE, DoubleBuilder);
-      BUILDER_CASE(STRING, StringBuilder);
-      BUILDER_CASE(BINARY, BinaryBuilder);
+    BUILDER_CASE(TIME32, Time32Builder);
+    BUILDER_CASE(TIME64, Time64Builder);
+    BUILDER_CASE(TIMESTAMP, TimestampBuilder);
+    BUILDER_CASE(BOOL, BooleanBuilder);
+    BUILDER_CASE(FLOAT, FloatBuilder);
+    BUILDER_CASE(DOUBLE, DoubleBuilder);
+    BUILDER_CASE(STRING, StringBuilder);
+    BUILDER_CASE(BINARY, BinaryBuilder);
     case Type::LIST: {
       std::shared_ptr<ArrayBuilder> value_builder;
       std::shared_ptr<DataType> value_type =

cpp/src/arrow/builder.h

@@ -231,7 +231,8 @@ using Int16Builder = NumericBuilder<Int16Type>;
 using Int32Builder = NumericBuilder<Int32Type>;
 using Int64Builder = NumericBuilder<Int64Type>;
 using TimestampBuilder = NumericBuilder<TimestampType>;
-using TimeBuilder = NumericBuilder<TimeType>;
+using Time32Builder = NumericBuilder<Time32Type>;
+using Time64Builder = NumericBuilder<Time64Type>;
 using Date32Builder = NumericBuilder<Date32Type>;
 using Date64Builder = NumericBuilder<Date64Type>;
 
@@ -378,6 +379,7 @@ class ARROW_EXPORT BinaryBuilder : public ListBuilder {
 // String builder
 class ARROW_EXPORT StringBuilder : public BinaryBuilder {
  public:
+  using BinaryBuilder::BinaryBuilder;
   explicit StringBuilder(MemoryPool* pool);
 
   using BinaryBuilder::Append;

cpp/src/arrow/compare.cc

@@ -29,6 +29,7 @@
 #include "arrow/type_traits.h"
 #include "arrow/util/bit-util.h"
 #include "arrow/util/logging.h"
+#include "arrow/visitor_inline.h"
 
 namespace arrow {
 
@@ -177,7 +178,13 @@ class RangeEqualsVisitor : public ArrayVisitor {
     return CompareValues<Date64Array>(left);
   }
 
-  Status Visit(const TimeArray& left) override { return CompareValues<TimeArray>(left); }
+  Status Visit(const Time32Array& left) override {
+    return CompareValues<Time32Array>(left);
+  }
+
+  Status Visit(const Time64Array& left) override {
+    return CompareValues<Time64Array>(left);
+  }
 
   Status Visit(const TimestampArray& left) override {
     return CompareValues<TimestampArray>(left);
@@ -415,7 +422,9 @@ class ArrayEqualsVisitor : public RangeEqualsVisitor {
 
   Status Visit(const Date64Array& left) override { return ComparePrimitive(left); }
 
-  Status Visit(const TimeArray& left) override { return ComparePrimitive(left); }
+  Status Visit(const Time32Array& left) override { return ComparePrimitive(left); }
+
+  Status Visit(const Time64Array& left) override { return ComparePrimitive(left); }
 
   Status Visit(const TimestampArray& left) override { return ComparePrimitive(left); }
 
@@ -628,7 +637,7 @@ Status ArrayApproxEquals(const Array& left, const Array& right, bool* are_equal)
 // ----------------------------------------------------------------------
 // Implement TypeEquals
 
-class TypeEqualsVisitor : public TypeVisitor {
+class TypeEqualsVisitor {
  public:
   explicit TypeEqualsVisitor(const DataType& right) : right_(right), result_(false) {}
 
@@ -648,29 +657,44 @@ class TypeEqualsVisitor : public TypeVisitor {
     return Status::OK();
   }
 
-  Status Visit(const TimeType& left) override {
-    const auto& right = static_cast<const TimeType&>(right_);
+  template <typename T>
+  typename std::enable_if<std::is_base_of<NoExtraMeta, T>::value ||
+                              std::is_base_of<PrimitiveCType, T>::value,
+      Status>::type
+  Visit(const T& type) {
+    result_ = true;
+    return Status::OK();
+  }
+
+  Status Visit(const Time32Type& left) {
+    const auto& right = static_cast<const Time32Type&>(right_);
     result_ = left.unit == right.unit;
     return Status::OK();
   }
 
-  Status Visit(const TimestampType& left) override {
+  Status Visit(const Time64Type& left) {
+    const auto& right = static_cast<const Time64Type&>(right_);
+    result_ = left.unit == right.unit;
+    return Status::OK();
+  }
+
+  Status Visit(const TimestampType& left) {
     const auto& right = static_cast<const TimestampType&>(right_);
     result_ = left.unit == right.unit && left.timezone == right.timezone;
     return Status::OK();
   }
 
-  Status Visit(const FixedWidthBinaryType& left) override {
+  Status Visit(const FixedWidthBinaryType& left) {
     const auto& right = static_cast<const FixedWidthBinaryType&>(right_);
     result_ = left.byte_width() == right.byte_width();
     return Status::OK();
   }
 
-  Status Visit(const ListType& left) override { return VisitChildren(left); }
+  Status Visit(const ListType& left) { return VisitChildren(left); }
 
-  Status Visit(const StructType& left) override { return VisitChildren(left); }
+  Status Visit(const StructType& left) { return VisitChildren(left); }
 
-  Status Visit(const UnionType& left) override {
+  Status Visit(const UnionType& left) {
     const auto& right = static_cast<const UnionType&>(right_);
 
     if (left.mode != right.mode || left.type_codes.size() != right.type_codes.size()) {
@@ -691,7 +715,7 @@ class TypeEqualsVisitor : public TypeVisitor {
     return Status::OK();
   }
 
-  Status Visit(const DictionaryType& left) override {
+  Status Visit(const DictionaryType& left) {
     const auto& right = static_cast<const DictionaryType&>(right_);
     result_ = left.index_type()->Equals(right.index_type()) &&
               left.dictionary()->Equals(right.dictionary());
@@ -713,18 +737,8 @@ Status TypeEquals(const DataType& left, const DataType& right, bool* are_equal)
     *are_equal = false;
   } else {
     TypeEqualsVisitor visitor(right);
-    Status s = left.Accept(&visitor);
-
-    // We do not implement any type visitors where there is no additional
-    // metadata to compare.
-    if (s.IsNotImplemented()) {
-      // Not implemented means there is no additional metadata to compare
-      *are_equal = true;
-    } else if (!s.ok()) {
-      return s;
-    } else {
-      *are_equal = visitor.result();
-    }
+    RETURN_NOT_OK(VisitTypeInline(left, &visitor));
+    *are_equal = visitor.result();
   }
   return Status::OK();
 }

cpp/src/arrow/ipc/feather-test.cc

@@ -353,7 +353,7 @@ TEST_F(TestTableWriter, CategoryRoundtrip) {
 TEST_F(TestTableWriter, TimeTypes) {
   std::vector<bool> is_valid = {true, true, true, false, true, true, true};
   auto f0 = field("f0", date32());
-  auto f1 = field("f1", time(TimeUnit::MILLI));
+  auto f1 = field("f1", time32(TimeUnit::MILLI));
   auto f2 = field("f2", timestamp(TimeUnit::NANO));
   auto f3 = field("f3", timestamp(TimeUnit::SECOND, "US/Los_Angeles"));
   std::shared_ptr<Schema> schema(new Schema({f0, f1, f2, f3}));

cpp/src/arrow/ipc/feather.cc

@@ -294,7 +294,7 @@ class TableReader::TableReaderImpl {
         break;
       case fbs::TypeMetadata_TimeMetadata: {
         auto meta = static_cast<const fbs::TimeMetadata*>(metadata);
-        *out = std::make_shared<TimeType>(FromFlatbufferEnum(meta->unit()));
+        *out = time32(FromFlatbufferEnum(meta->unit()));
       } break;
       default:
         switch (values->type()) {
@@ -476,7 +476,9 @@ fbs::Type ToFlatbufferType(Type::type type) {
       return fbs::Type_DATE;
     case Type::TIMESTAMP:
       return fbs::Type_TIMESTAMP;
-    case Type::TIME:
+    case Type::TIME32:
+      return fbs::Type_TIME;
+    case Type::TIME64:
       return fbs::Type_TIME;
     case Type::DICTIONARY:
       return fbs::Type_CATEGORY;
@@ -646,13 +648,17 @@ class TableWriter::TableWriterImpl : public ArrayVisitor {
     return Status::OK();
   }
 
-  Status Visit(const TimeArray& values) override {
+  Status Visit(const Time32Array& values) override {
     RETURN_NOT_OK(WritePrimitiveValues(values));
-    auto unit = static_cast<const TimeType&>(*values.type()).unit;
+    auto unit = static_cast<const Time32Type&>(*values.type()).unit;
     current_column_->SetTime(unit);
     return Status::OK();
   }
 
+  Status Visit(const Time64Array& values) override {
+    return Status::NotImplemented("time64");
+  }
+
   Status Append(const std::string& name, const Array& values) {
     current_column_ = metadata_.AddColumn(name);
     RETURN_NOT_OK(values.Accept(this));

cpp/src/arrow/ipc/ipc-json-test.cc

@@ -52,7 +52,9 @@ void TestSchemaRoundTrip(const Schema& schema) {
   std::shared_ptr<Schema> out;
   ASSERT_OK(ReadJsonSchema(d, &out));
 
-  ASSERT_TRUE(schema.Equals(out));
+  if (!schema.Equals(out)) {
+    FAIL() << "In schema: " << schema.ToString() << "\nOut schema: " << out->ToString();
+  }
 }
 
 void TestArrayRoundTrip(const Array& array) {
@@ -105,8 +107,8 @@ TEST(TestJsonSchemaWriter, FlatTypes) {
       field("f10", utf8()), field("f11", binary()), field("f12", list(int32())),
       field("f13", struct_({field("s1", int32()), field("s2", utf8())})),
       field("f15", date64()), field("f16", timestamp(TimeUnit::NANO)),
-      field("f17", time(TimeUnit::MICRO)),
-      field("f18", union_({field("u1", int8()), field("u2", time(TimeUnit::MILLI))},
+      field("f17", time64(TimeUnit::MICRO)),
+      field("f18", union_({field("u1", int8()), field("u2", time32(TimeUnit::MILLI))},
                        {0, 1}, UnionMode::DENSE))};
 
   Schema schema(fields);