fix review and add more tests

Author: HuaHuaY

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

@@ -28,6 +28,7 @@
 #include <functional>
 #include <set>
 #include <sstream>
+#include <utility>
 #include <vector>
 
 #include "arrow/array/builder_binary.h"
@@ -376,19 +377,19 @@ const double test_traits<::arrow::DoubleType>::value(4.2);
 template <>
 struct test_traits<::arrow::StringType> {
   static constexpr ParquetType::type parquet_enum = ParquetType::BYTE_ARRAY;
-  static std::string const value;
+  static const std::string value;
 };
 
 template <>
 struct test_traits<::arrow::BinaryType> {
   static constexpr ParquetType::type parquet_enum = ParquetType::BYTE_ARRAY;
-  static std::string const value;
+  static const std::string value;
 };
 
 template <>
 struct test_traits<::arrow::FixedSizeBinaryType> {
   static constexpr ParquetType::type parquet_enum = ParquetType::FIXED_LEN_BYTE_ARRAY;
-  static std::string const value;
+  static const std::string value;
 };
 
 const std::string test_traits<::arrow::StringType>::value("Test");            // NOLINT
@@ -4150,20 +4151,23 @@ INSTANTIATE_TEST_SUITE_P(Repetition_type, TestNestedSchemaRead,
                          ::testing::Values(Repetition::REQUIRED, Repetition::OPTIONAL));
 
 TEST(TestImpalaConversion, ArrowTimestampToImpalaTimestamp) {
-  {
-    // June 20, 2017 16:32:56 and 123456789 nanoseconds
-    int64_t timestamp = INT64_C(1497976376123456789);
-    Int96 impala_timestamp = {{UINT32_C(632093973), UINT32_C(13871), UINT32_C(2457925)}};
-    ASSERT_EQ(timestamp, ::parquet::Int96GetNanoSeconds(impala_timestamp));
+  std::vector<std::pair<int64_t, Int96>> test_cases = {
+      // June 20, 2017 16:32:56 and 123456789 nanoseconds
+      {INT64_C(1497976376123456789),
+       {{UINT32_C(632093973), UINT32_C(13871), UINT32_C(2457925)}}},
+      // January 1, 1970 00:00:00 and 000000000 nanoseconds
+      {INT64_C(0), {{UINT32_C(0), UINT32_C(0), UINT32_C(2440588)}}},
+      // December 31, 1969 23:59:59 and 999999000 nanoseconds
+      {INT64_C(-1000), {{UINT32_C(2437872664), UINT32_C(20116), UINT32_C(2440587)}}},
+      // December 31, 1969 00:00:00 and 000000000 nanoseconds
+      {INT64_C(-86400000000000), {{UINT32_C(0), UINT32_C(0), UINT32_C(2440587)}}},
+      // January 1, 1970 00:00:00 and 000001000 nanoseconds
+      {INT64_C(1000), {{UINT32_C(1000), UINT32_C(0), UINT32_C(2440588)}}},
+      // January 2, 1970 00:00:00 and 000000000 nanoseconds
+      {INT64_C(86400000000000), {{UINT32_C(0), UINT32_C(0), UINT32_C(2440589)}}},
+  };
 
-    Int96 calculated;
-    ::parquet::internal::NanosecondsToImpalaTimestamp(timestamp, &calculated);
-    ASSERT_EQ(impala_timestamp, calculated);
-  }
-  {
-    // January 1, 1970 07:59:59 and 999999000 nanoseconds
-    int64_t timestamp = INT64_C(-1000);
-    Int96 impala_timestamp = {{UINT32_C(2437872664), UINT32_C(20116), UINT32_C(2440587)}};
+  for (auto& [timestamp, impala_timestamp] : test_cases) {
     ASSERT_EQ(timestamp, ::parquet::Int96GetNanoSeconds(impala_timestamp));
 
     Int96 calculated;

cpp/src/parquet/column_writer.h

@@ -259,12 +259,14 @@ constexpr int64_t kJulianEpochOffsetDays = INT64_C(2440588);
 
 template <int64_t UnitPerDay, int64_t NanosecondsPerUnit>
 inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_timestamp) {
-  int32_t julian_days = static_cast<int32_t>(time / UnitPerDay + kJulianEpochOffsetDays) +
-                        (time < 0 ? -1 : 0);
+  auto julian_days = static_cast<int32_t>(time / UnitPerDay + kJulianEpochOffsetDays);
+  int64_t last_day_units = time % UnitPerDay;
+  if (last_day_units < 0) {
+    --julian_days;
+    last_day_units += UnitPerDay;
+  }
   impala_timestamp->value[2] = static_cast<uint32_t>(julian_days);
-
-  uint64_t last_day_units = time % UnitPerDay + (time < 0 ? UnitPerDay : 0);
-  uint64_t last_day_nanos = last_day_units * NanosecondsPerUnit;
+  uint64_t last_day_nanos = static_cast<uint64_t>(last_day_units) * NanosecondsPerUnit;
   // impala_timestamp will be unaligned every other entry so do memcpy instead
   // of assign and reinterpret cast to avoid undefined behavior.
   std::memcpy(impala_timestamp, &last_day_nanos, sizeof(uint64_t));