address review

Author: HuaHuaY

cpp/src/parquet/file_rewriter.cc

@@ -470,7 +470,7 @@ class SingleFileRewriter {
     if (current_row_group_index_ >= metadata_->num_row_groups()) {
       throw ParquetException("Trying to read row group ", current_row_group_index_,
                              " but file only has ", metadata_->num_row_groups(),
-                             " row group");
+                             " row groups");
     }
     auto row_group_reader = parquet_file_reader_->RowGroup(current_row_group_index_);
     auto page_index_reader = page_index_reader_
@@ -771,7 +771,7 @@ class GeneratedFile : public ParquetFileRewriter::Contents {
 };
 
 // ----------------------------------------------------------------------
-// ParquetFilesRewriter public API
+// ParquetFileRewriter public API
 
 ParquetFileRewriter::ParquetFileRewriter() = default;
 

cpp/src/parquet/metadata.cc

@@ -18,7 +18,6 @@
 #include "parquet/metadata.h"
 
 #include <algorithm>
-#include <cinttypes>
 #include <memory>
 #include <ostream>
 #include <random>
@@ -29,7 +28,6 @@
 #include <vector>
 
 #include "arrow/io/memory.h"
-#include "arrow/util/key_value_metadata.h"
 #include "arrow/util/logging_internal.h"
 #include "arrow/util/pcg_random.h"
 #include "parquet/encryption/encryption_internal.h"
@@ -165,40 +163,6 @@ std::shared_ptr<Statistics> MakeColumnStats(const format::ColumnMetaData& meta_d
   }
   throw ParquetException("Can't decode page statistics for selected column type");
 }
-
-// Get KeyValueMetadata from parquet Thrift RowGroup or ColumnChunk metadata.
-//
-// Returns nullptr if the metadata is not set.
-template <typename Metadata>
-std::shared_ptr<KeyValueMetadata> FromThriftKeyValueMetadata(const Metadata& source) {
-  std::shared_ptr<KeyValueMetadata> metadata = nullptr;
-  if (source.__isset.key_value_metadata) {
-    std::vector<std::string> keys;
-    std::vector<std::string> values;
-    keys.reserve(source.key_value_metadata.size());
-    values.reserve(source.key_value_metadata.size());
-    for (const auto& it : source.key_value_metadata) {
-      keys.push_back(it.key);
-      values.push_back(it.value);
-    }
-    metadata = std::make_shared<KeyValueMetadata>(std::move(keys), std::move(values));
-  }
-  return metadata;
-}
-
-template <typename Metadata>
-void ToThriftKeyValueMetadata(const KeyValueMetadata& source, Metadata* metadata) {
-  std::vector<format::KeyValue> key_value_metadata;
-  key_value_metadata.reserve(static_cast<size_t>(source.size()));
-  for (int64_t i = 0; i < source.size(); ++i) {
-    format::KeyValue kv_pair;
-    kv_pair.__set_key(source.key(i));
-    kv_pair.__set_value(source.value(i));
-    key_value_metadata.emplace_back(std::move(kv_pair));
-  }
-  metadata->__set_key_value_metadata(std::move(key_value_metadata));
-}
-
 }  // namespace
 
 // MetaData Accessor
@@ -460,8 +424,6 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
     return key_value_metadata_;
   }
 
-  const void* to_thrift() const { return column_metadata_; }
-
  private:
   void InitKeyValueMetadata() {
     key_value_metadata_ = FromThriftKeyValueMetadata(*column_metadata_);
@@ -613,8 +575,6 @@ const std::shared_ptr<const KeyValueMetadata>& ColumnChunkMetaData::key_value_me
   return impl_->key_value_metadata();
 }
 
-const void* ColumnChunkMetaData::to_thrift() const { return impl_->to_thrift(); }
-
 // row-group metadata
 class RowGroupMetaData::RowGroupMetaDataImpl {
  public:
@@ -1948,8 +1908,7 @@ class RowGroupMetaDataBuilder::RowGroupMetaDataBuilderImpl {
     auto* column_chunk = &row_group_->columns[next_column_++];
     column_chunk->__set_file_offset(0);
     column_chunk->__isset.meta_data = true;
-    column_chunk->meta_data =
-        *static_cast<const format::ColumnMetaData*>(cc_metadata->to_thrift());
+    column_chunk->meta_data = ToThrift(*cc_metadata);
 
     auto& meta_data = column_chunk->meta_data;
     meta_data.__set_data_page_offset(meta_data.data_page_offset + shift);

cpp/src/parquet/metadata.h

@@ -162,8 +162,6 @@ class PARQUET_EXPORT ColumnChunkMetaData {
   std::optional<IndexLocation> GetOffsetIndexLocation() const;
   const std::shared_ptr<const KeyValueMetadata>& key_value_metadata() const;
 
-  const void* to_thrift() const;
-
  private:
   explicit ColumnChunkMetaData(
       const void* metadata, const ColumnDescriptor* descr, int16_t row_group_ordinal,

cpp/src/parquet/thrift_internal.h

@@ -36,12 +36,14 @@
 #include <thrift/protocol/TCompactProtocol.h>
 #include <thrift/transport/TBufferTransports.h>
 
+#include "arrow/util/key_value_metadata.h"  // IWYU pragma: keep
 #include "arrow/util/logging.h"
 
 #include "parquet/encryption/internal_file_decryptor.h"
 #include "parquet/encryption/internal_file_encryptor.h"
 #include "parquet/exception.h"
 #include "parquet/geospatial/statistics.h"
+#include "parquet/metadata.h"
 #include "parquet/page_index.h"
 #include "parquet/platform.h"
 #include "parquet/properties.h"
@@ -431,6 +433,19 @@ constexpr format::BoundaryOrder::type ToThrift(BoundaryOrder::type type) {
   }
 }
 
+constexpr format::PageType::type ToThrift(PageType::type type) {
+  switch (type) {
+    case PageType::DATA_PAGE:
+    case PageType::INDEX_PAGE:
+    case PageType::DICTIONARY_PAGE:
+    case PageType::DATA_PAGE_V2:
+      return static_cast<format::PageType::type>(type);
+    default:
+      ARROW_DCHECK(false) << "Cannot reach here";
+      return format::PageType::DATA_PAGE;
+  }
+}
+
 static inline format::SortingColumn ToThrift(SortingColumn sorting_column) {
   format::SortingColumn thrift_sorting_column;
   thrift_sorting_column.column_idx = sorting_column.column_idx;
@@ -545,6 +560,85 @@ static inline format::SizeStatistics ToThrift(const SizeStatistics& size_stats)
   return size_statistics;
 }
 
+// Get KeyValueMetadata from parquet Thrift RowGroup or ColumnChunk metadata.
+//
+// Returns nullptr if the metadata is not set.
+template <typename Metadata>
+std::shared_ptr<KeyValueMetadata> FromThriftKeyValueMetadata(const Metadata& source) {
+  std::shared_ptr<KeyValueMetadata> metadata = nullptr;
+  if (source.__isset.key_value_metadata) {
+    std::vector<std::string> keys;
+    std::vector<std::string> values;
+    keys.reserve(source.key_value_metadata.size());
+    values.reserve(source.key_value_metadata.size());
+    for (const auto& it : source.key_value_metadata) {
+      keys.push_back(it.key);
+      values.push_back(it.value);
+    }
+    metadata = std::make_shared<KeyValueMetadata>(std::move(keys), std::move(values));
+  }
+  return metadata;
+}
+
+template <typename Metadata>
+void ToThriftKeyValueMetadata(const KeyValueMetadata& source, Metadata* metadata) {
+  std::vector<format::KeyValue> key_value_metadata;
+  key_value_metadata.reserve(static_cast<size_t>(source.size()));
+  for (int64_t i = 0; i < source.size(); ++i) {
+    format::KeyValue kv_pair;
+    kv_pair.__set_key(source.key(i));
+    kv_pair.__set_value(source.value(i));
+    key_value_metadata.emplace_back(std::move(kv_pair));
+  }
+  metadata->__set_key_value_metadata(std::move(key_value_metadata));
+}
+
+static inline format::ColumnMetaData ToThrift(const ColumnChunkMetaData& cc_metadata) {
+  format::ColumnMetaData column_meta_data;
+  column_meta_data.__set_type(ToThrift(cc_metadata.type()));
+  std::vector<format::Encoding::type> thrift_encodings;
+  thrift_encodings.reserve(cc_metadata.encodings().size());
+  for (const auto& encoding : cc_metadata.encodings()) {
+    thrift_encodings.push_back(ToThrift(encoding));
+  }
+  column_meta_data.__set_encodings(std::move(thrift_encodings));
+  column_meta_data.__set_path_in_schema(cc_metadata.path_in_schema()->ToDotVector());
+  column_meta_data.__set_codec(ToThrift(cc_metadata.compression()));
+  column_meta_data.__set_num_values(cc_metadata.num_values());
+  column_meta_data.__set_total_uncompressed_size(cc_metadata.total_uncompressed_size());
+  column_meta_data.__set_total_compressed_size(cc_metadata.total_compressed_size());
+  ToThriftKeyValueMetadata(*cc_metadata.key_value_metadata(), &column_meta_data);
+  column_meta_data.__set_data_page_offset(cc_metadata.data_page_offset());
+  if (cc_metadata.has_index_page()) {
+    column_meta_data.__set_index_page_offset(cc_metadata.index_page_offset());
+  }
+  if (cc_metadata.has_dictionary_page()) {
+    column_meta_data.__set_dictionary_page_offset(cc_metadata.dictionary_page_offset());
+  }
+  if (cc_metadata.is_stats_set()) {
+    column_meta_data.__set_statistics(ToThrift(*cc_metadata.encoded_statistics()));
+  }
+  std::vector<format::PageEncodingStats> thrift_encoding_stats;
+  thrift_encoding_stats.reserve(cc_metadata.encoding_stats().size());
+  for (const auto& encoding_stat : cc_metadata.encoding_stats()) {
+    format::PageEncodingStats enc_stat;
+    enc_stat.__set_page_type(ToThrift(encoding_stat.page_type));
+    enc_stat.__set_encoding(ToThrift(encoding_stat.encoding));
+    enc_stat.__set_count(encoding_stat.count);
+    thrift_encoding_stats.push_back(enc_stat);
+  }
+  column_meta_data.__set_encoding_stats(std::move(thrift_encoding_stats));
+  if (auto size_stats = cc_metadata.size_statistics(); size_stats != nullptr) {
+    column_meta_data.__set_size_statistics(ToThrift(*size_stats));
+  }
+  if (auto geo_stats = cc_metadata.geo_statistics(); geo_stats != nullptr) {
+    if (auto encoded_geo_stats = geo_stats->Encode(); encoded_geo_stats.has_value()) {
+      column_meta_data.__set_geospatial_statistics(ToThrift(*encoded_geo_stats));
+    }
+  }
+  return column_meta_data;
+}
+
 static inline format::PageLocation ToThrift(const PageLocation& page_location) {
   format::PageLocation thrift_page_location;
   thrift_page_location.__set_offset(page_location.offset);