From 72e67b31e81b0feae630539ce4be79bd4ae45db6 Mon Sep 17 00:00:00 2001
From: Kapil Singh <kapilsingh@microsoft.com>
Date: Fri, 12 Jan 2024 11:19:38 +0530
Subject: [PATCH] Resolve comments

---
 velox/dwio/common/TimestampDecoder.h        | 15 ++++++---------
 velox/dwio/parquet/reader/PageReader.cpp    |  4 ++--
 velox/dwio/parquet/reader/ParquetReader.cpp |  2 +-
 velox/type/TimestampConversion.h            |  4 ++--
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/velox/dwio/common/TimestampDecoder.h b/velox/dwio/common/TimestampDecoder.h
index 6b97b90cf6b0b..af8ab334868e1 100644
--- a/velox/dwio/common/TimestampDecoder.h
+++ b/velox/dwio/common/TimestampDecoder.h
@@ -31,7 +31,7 @@ class TimestampDecoder : public DirectDecoder<false> {
       uint32_t numBytes,
       bool bigEndian = false)
       : DirectDecoder<false>{std::move(input), useVInts, numBytes, bigEndian},
-        _precision(precision) {}
+        precision_(precision) {}
 
   template <bool hasNulls, typename Visitor>
   void readWithVisitor(
@@ -62,15 +62,12 @@ class TimestampDecoder : public DirectDecoder<false> {
       }
       if constexpr (std::is_same_v<typename Visitor::DataType, int128_t>) {
         auto units = IntDecoder<false>::template readInt<int64_t>();
-        Timestamp timestap;
-        if (_precision == TIMESTAMP_PRECISION::MILLIS) {
-          timestap = facebook::velox::util::fromUTCMillisParquet(units);
-        } else {
-          timestap = facebook::velox::util::fromUTCMicrosParquet(units);
-        }
+        Timestamp timestamp = precision_ == TIMESTAMP_PRECISION::MILLIS
+            ? util::fromUTCMillis(units)
+            : util::fromUTCMicros(units);
 
         toSkip =
-            visitor.process(*reinterpret_cast<int128_t*>(&timestap), atEnd);
+            visitor.process(*reinterpret_cast<int128_t*>(&timestamp), atEnd);
       } else {
         toSkip = visitor.process(
             IntDecoder<false>::template readInt<int64_t>(), atEnd);
@@ -88,6 +85,6 @@ class TimestampDecoder : public DirectDecoder<false> {
   }
 
  private:
-  TIMESTAMP_PRECISION _precision;
+  TIMESTAMP_PRECISION precision_;
 };
 } // namespace facebook::velox::dwio::common
diff --git a/velox/dwio/parquet/reader/PageReader.cpp b/velox/dwio/parquet/reader/PageReader.cpp
index cfef9325f93f1..30b3dd68bcfed 100644
--- a/velox/dwio/parquet/reader/PageReader.cpp
+++ b/velox/dwio/parquet/reader/PageReader.cpp
@@ -409,12 +409,12 @@ void PageReader::prepareDictionary(const PageHeader& pageHeader) {
           if (logicalType.TIMESTAMP.unit.__isset.MICROS) {
             for (auto i = dictionary_.numValues - 1; i >= 0; --i) {
               memcpy(&units, parquetValues + i * typeSize, typeSize);
-              values[i] = facebook::velox::util::fromUTCMicrosParquet(units);
+              values[i] = util::fromUTCMicros(units);
             }
           } else if (logicalType.TIMESTAMP.unit.__isset.MILLIS) {
             for (auto i = dictionary_.numValues - 1; i >= 0; --i) {
               memcpy(&units, parquetValues + i * typeSize, typeSize);
-              values[i] = facebook::velox::util::fromUTCMillisParquet(units);
+              values[i] = util::fromUTCMillis(units);
             }
           } else {
             VELOX_NYI("Unsupported timestamp unit");
diff --git a/velox/dwio/parquet/reader/ParquetReader.cpp b/velox/dwio/parquet/reader/ParquetReader.cpp
index cfa2bfc95ca8c..3a753f843e1bc 100644
--- a/velox/dwio/parquet/reader/ParquetReader.cpp
+++ b/velox/dwio/parquet/reader/ParquetReader.cpp
@@ -395,7 +395,7 @@ std::shared_ptr<const ParquetTypeWithId> ReaderBase::getParquetColumnInfo(
     if (veloxType->kind() == TypeKind::TIMESTAMP &&
         schemaElement.type == thrift::Type::INT64 &&
         !logicalType_.has_value()) {
-      // Construct logical type from deprecated converted type of Parquet
+      // Construct logical type from deprecated converted type of Parquet.
       thrift::TimestampType timestamp;
       timestamp.__set_isAdjustedToUTC(true);
       thrift::TimeUnit unit;
diff --git a/velox/type/TimestampConversion.h b/velox/type/TimestampConversion.h
index 0b166e2f2df48..e7f13bc796042 100644
--- a/velox/type/TimestampConversion.h
+++ b/velox/type/TimestampConversion.h
@@ -134,11 +134,11 @@ inline Timestamp fromTimestampString(const StringView& str) {
 
 Timestamp fromDatetime(int64_t daysSinceEpoch, int64_t microsSinceMidnight);
 
-inline Timestamp fromUTCMillisParquet(int64_t millis) {
+inline Timestamp fromUTCMillis(int64_t millis) {
   return Timestamp(millis / 1000, (millis % 1000) * 1000000);
 }
 
-inline Timestamp fromUTCMicrosParquet(int64_t micros) {
+inline Timestamp fromUTCMicros(int64_t micros) {
   return Timestamp(micros / 1000000, (micros % 1000000) * 1000);
 }