Some review feedback

rok · rok · commit 8c94e80d6208 · 2026-03-06T00:52:06.000+01:00
diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt
@@ -309,9 +309,9 @@ add_arrow_lib(parquet
               STATIC_LINK_LIBS
               ${PARQUET_STATIC_LINK_LIBS}
               STATIC_INSTALL_INTERFACE_LIBS
-              ${PARQUET_STATIC_INSTALL_INTERFACE_LIBS})
-
-target_include_directories(parquet_objlib SYSTEM PRIVATE ${ARROW_SOURCE_DIR}/thirdparty/flatbuffers/include)
+              ${PARQUET_STATIC_INSTALL_INTERFACE_LIBS}
+              PRIVATE_INCLUDES
+              ${ARROW_SOURCE_DIR}/thirdparty/flatbuffers/include)
 
 if(WIN32 AND NOT (ARROW_TEST_LINKAGE STREQUAL "static"))
   add_library(parquet_test_support STATIC
diff --git a/cpp/src/parquet/file_reader.cc b/cpp/src/parquet/file_reader.cc
@@ -441,13 +441,13 @@ class SerializedFile : public ParquetFileReader::Contents {
     PARQUET_ASSIGN_OR_THROW(
         auto footer_buffer,
         source_->ReadAt(source_size_ - footer_read_size, footer_read_size));
-    if (properties_.read_metadata3()) {
+    if (properties_.read_flatbuffer_metadata_if_present()) {
       // Try to extract flatbuffer metadata from footer
       std::string flatbuffer_data;
       auto result = ExtractFlatbuffer(footer_buffer, &flatbuffer_data);
       if (result.ok()) {
-        int32_t required_or_consumed = *result;
-        if (required_or_consumed > static_cast<int32_t>(footer_buffer->size())) {
+        uint32_t required_or_consumed = *result;
+        if (required_or_consumed > static_cast<uint32_t>(footer_buffer->size())) {
           PARQUET_ASSIGN_OR_THROW(
               footer_buffer,
               source_->ReadAt(source_size_ - required_or_consumed, required_or_consumed));
@@ -461,8 +461,8 @@ class SerializedFile : public ParquetFileReader::Contents {
               format3::GetFileMetaData(flatbuffer_data.data());
           auto thrift_metadata =
               std::make_unique<format::FileMetaData>(FromFlatbuffer(fb_metadata));
-          file_metadata_ = FileMetaData::Make(
-              std::move(thrift_metadata), static_cast<uint32_t>(*result), properties_);
+          file_metadata_ =
+              FileMetaData::Make(std::move(thrift_metadata), *result, properties_);
           return;
         }
       }
diff --git a/cpp/src/parquet/file_writer.h b/cpp/src/parquet/file_writer.h
@@ -110,8 +110,7 @@ class PARQUET_EXPORT RowGroupWriter {
 };
 
 PARQUET_EXPORT
-void WriteFileMetaData(const FileMetaData& file_metadata,
-                       ::arrow::io::OutputStream* sink,
+void WriteFileMetaData(const FileMetaData& file_metadata, ::arrow::io::OutputStream* sink,
                        bool use_metadata3 = false);
 
 PARQUET_EXPORT
diff --git a/cpp/src/parquet/metadata.h b/cpp/src/parquet/metadata.h
@@ -25,12 +25,12 @@
 #include <string>
 #include <vector>
 
+#include "generated/parquet_types.h"
 #include "parquet/encryption/type_fwd.h"
 #include "parquet/index_location.h"
 #include "parquet/platform.h"
 #include "parquet/properties.h"
 #include "parquet/type_fwd.h"
-#include "generated/parquet_types.h"
 
 namespace parquet {
 
@@ -389,8 +389,8 @@ class PARQUET_EXPORT FileMetaData {
                         const ReaderProperties& properties,
                         std::shared_ptr<InternalFileDecryptor> file_decryptor = NULLPTR);
 
-  explicit FileMetaData(std::unique_ptr<format::FileMetaData> metadata, uint32_t metadata_len,
-                        const ReaderProperties& properties);
+  explicit FileMetaData(std::unique_ptr<format::FileMetaData> metadata,
+                        uint32_t metadata_len, const ReaderProperties& properties);
 
   void set_file_decryptor(std::shared_ptr<InternalFileDecryptor> file_decryptor);
   const std::shared_ptr<InternalFileDecryptor>& file_decryptor() const;
diff --git a/cpp/src/parquet/metadata3.cc b/cpp/src/parquet/metadata3.cc
@@ -21,13 +21,14 @@
 #include <string>
 #include <vector>
 
+#include "arrow/util/bit_util.h"
 #include "arrow/util/compression.h"
 #include "arrow/util/crc32.h"
 #include "arrow/util/endian.h"
 #include "arrow/util/ubsan.h"
 #include "arrow/util/unreachable.h"
-#include "parquet/file_writer.h"
 #include "generated/parquet_types.h"
+#include "parquet/file_writer.h"
 #include "parquet/thrift_internal.h"
 
 namespace parquet {
@@ -92,30 +93,35 @@ static_assert(IsEnumEq(format::PageType::DICTIONARY_PAGE,
 constexpr double kMinCompressionRatio = 1.2;
 
 constexpr uint8_t kExtUUID[16] = {0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef,
-                                   0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef};
+                                  0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef};
 
 // Extended format compression codec (using same values as format3::CompressionCodec)
 enum class CompressionCodec : uint8_t {
   UNCOMPRESSED = 0,
   LZ4_RAW = 7,
 };
 
-auto GetNumChildren(
+int32_t GetNumChildren(
     const flatbuffers::Vector<flatbuffers::Offset<format3::SchemaElement>>& s, size_t i) {
   return s.Get(i)->num_children();
 }
 
-auto GetNumChildren(const std::vector<format::SchemaElement>& s, size_t i) {
+int32_t GetNumChildren(const std::vector<format::SchemaElement>& s, size_t i) {
   return s[i].num_children;
 }
 
-auto GetName(const flatbuffers::Vector<flatbuffers::Offset<format3::SchemaElement>>& s,
-             size_t i) {
+std::string GetName(
+    const flatbuffers::Vector<flatbuffers::Offset<format3::SchemaElement>>& s, size_t i) {
   return s.Get(i)->name()->str();
 }
 
-auto GetName(const std::vector<format::SchemaElement>& s, size_t i) { return s[i].name; }
+std::string GetName(const std::vector<format::SchemaElement>& s, size_t i) {
+  return s[i].name;
+}
 
+// Maps between column chunk indices (leaf columns only) and schema element indices
+// (all columns including groups). Also tracks parent relationships for building
+// column paths.
 class ColumnMap {
  public:
   template <typename Schema>
@@ -126,7 +132,9 @@ class ColumnMap {
     BuildParents(s);
   }
 
+  // Convert a column chunk index to the corresponding schema element index.
   size_t ToSchema(size_t cc_idx) const { return colchunk2schema_[cc_idx]; }
+  // Convert a schema element index to its column chunk index, if it is a leaf column.
   std::optional<size_t> ToCc(size_t schema_idx) const {
     auto it =
         std::lower_bound(colchunk2schema_.begin(), colchunk2schema_.end(), schema_idx);
@@ -177,6 +185,9 @@ class ColumnMap {
   std::vector<uint32_t> parents_;
 };
 
+// Packed representation of min/max statistics for a column chunk.
+// Values are split into lo4 (4 bytes), lo8 (8 bytes), and hi8 (8 bytes) parts
+// to allow compact flatbuffer encoding.
 struct MinMax {
   struct Packed {
     uint32_t lo4 = 0;
@@ -1137,80 +1148,62 @@ static std::string PackFlatbuffer(const std::string& in) {
   uint8_t* const p = reinterpret_cast<uint8_t*>(out.data()) + n + 1;
 
   // Compute and store checksums and lengths
-  uint32_t crc32 = ::arrow::internal::crc32(0, reinterpret_cast<const uint8_t*>(out.data()), n + 1);
-  StoreLE32(crc32, p + 0);           // crc32(data .. compressor)
-  StoreLE32(n, p + 4);               // compressed_len
-  StoreLE32(in.size(), p + 8);       // raw_len
+  uint32_t crc32 =
+      ::arrow::internal::crc32(0, reinterpret_cast<const uint8_t*>(out.data()), n + 1);
+  StoreLE32(crc32, p + 0);      // crc32(data .. compressor)
+  StoreLE32(n, p + 4);          // compressed_len
+  StoreLE32(in.size(), p + 8);  // raw_len
   uint32_t len_crc32 = ::arrow::internal::crc32(0, p + 4, 8);
-  StoreLE32(len_crc32, p + 12);      // crc32(compressed_len .. raw_len)
+  StoreLE32(len_crc32, p + 12);  // crc32(compressed_len .. raw_len)
 
   // Store UUID identifier
   std::memcpy(p + 16, kExtUUID, 16);
   out.resize(n + 33);
   return out;
 }
 
-inline uint8_t* WriteULEB64(uint64_t v, uint8_t* out) {
-  uint8_t* p = out;
-  do {
-    uint8_t b = v & 0x7F;
-    if (v < 0x80) {
-      *p++ = b;
-      return p;
-    }
-    *p++ = b | 0x80;
-    v >>= 7;
-  } while (true);
-}
-
-inline uint32_t CountLeadingZeros32(uint32_t v) {
-  if (v == 0) return 32;
-  uint32_t count = 0;
-  uint32_t mask = 0x80000000;
-  while ((v & mask) == 0) {
-    ++count;
-    mask >>= 1;
-  }
-  return count;
-}
-
-inline int32_t ULEB32Len(uint32_t v) {
-  return 1 + ((32 - CountLeadingZeros32(v | 0x1)) * 9) / 64;
-}
-
 void AppendFlatbuffer(std::string flatbuffer, std::string* thrift) {
+  using ::arrow::bit_util::kMaxLEB128ByteLenFor;
+  using ::arrow::bit_util::WriteLEB128;
+
   // Pack the flatbuffer with LZ4 compression and checksums
   std::string packed = PackFlatbuffer(flatbuffer);
 
   const uint32_t kFieldId = 32767;
-  int header_size = 1 + ULEB32Len(kFieldId) + ULEB32Len(packed.length());
+  // Max header: 1 (type byte) + max ULEB for field id + max ULEB for packed length
+  constexpr int32_t kMaxHeaderSize =
+      1 + kMaxLEB128ByteLenFor<uint32_t> + kMaxLEB128ByteLenFor<uint32_t>;
 
   const size_t old_size = thrift->size();
-  thrift->resize(old_size + header_size + packed.size() + 1);  // +1 for stop field
+  thrift->resize(old_size + kMaxHeaderSize + packed.size() + 1);  // +1 for stop field
 
   // Pointer to the new write position
   uint8_t* p = reinterpret_cast<uint8_t*>(&(*thrift)[old_size]);
 
   // Write the binary type indicator
   *p++ = 0x08;
 
-  // Write field id and size using ULEB64
-  p = WriteULEB64(kFieldId, p);
-  p = WriteULEB64(static_cast<uint32_t>(packed.size()), p);
+  // Write field id and size using ULEB128
+  p += WriteLEB128(kFieldId, p, kMaxLEB128ByteLenFor<uint32_t>);
+  p += WriteLEB128(static_cast<uint32_t>(packed.size()), p,
+                   kMaxLEB128ByteLenFor<uint32_t>);
 
   // Copy the packed payload
   std::memcpy(p, packed.data(), packed.size());
   p += packed.size();
 
   // Add stop field
   *p = 0x00;
-  return;
+
+  // Trim to actual size (header may have been smaller than max)
+  thrift->resize(p - reinterpret_cast<uint8_t*>(thrift->data()) + 1);
 }
 
-::arrow::Result<int32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf, std::string* out_flatbuffer) {
+::arrow::Result<uint32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf,
+                                            std::string* out_flatbuffer) {
   if (buf->size() < 8) return 8;
   PARQUET_THROW_NOT_OK(CheckMagicNumber(buf->data() + buf->size() - 4));
-  uint32_t md_len = LoadLE32(buf->data() + buf->size() -8);
+  uint32_t md_len = LoadLE32(buf->data() + buf->size() - 8);
   if (md_len < 34) return 0;
   if (buf->size() < 42) return 42;  // 34 (metadata3 trailer) + 8 (len + PAR1)
 
@@ -1235,7 +1228,8 @@ ::arrow::Result<int32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf, std::str
   }
 
   // Verify data CRC
-  uint32_t expected_crc = ::arrow::internal::crc32(0, p - compressed_len, compressed_len + 1);
+  uint32_t expected_crc =
+      ::arrow::internal::crc32(0, p - compressed_len, compressed_len + 1);
   if (crc32_val != expected_crc) {
     return ::arrow::Status::Invalid("Extended metadata data CRC mismatch");
   }
@@ -1254,11 +1248,11 @@ ::arrow::Result<int32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf, std::str
         return ::arrow::Status::Invalid("LZ4 length error: raw_len < compressed_len");
       }
       // Use Arrow's LZ4 codec for decompression
-      ARROW_ASSIGN_OR_RAISE(auto codec, ::arrow::util::Codec::Create(::arrow::Compression::LZ4));
-      ARROW_ASSIGN_OR_RAISE(
-          int64_t actual_size,
-          codec->Decompress(compressed_len, p - compressed_len, raw_len,
-                            decompressed_data.data()));
+      ARROW_ASSIGN_OR_RAISE(auto codec,
+                            ::arrow::util::Codec::Create(::arrow::Compression::LZ4));
+      ARROW_ASSIGN_OR_RAISE(int64_t actual_size,
+                            codec->Decompress(compressed_len, p - compressed_len, raw_len,
+                                              decompressed_data.data()));
       if (static_cast<uint32_t>(actual_size) != raw_len) {
         return ::arrow::Status::Invalid("LZ4 decompression failed: expected ", raw_len,
                                         " bytes but got ", actual_size, " bytes");
@@ -1276,7 +1270,8 @@ ::arrow::Result<int32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf, std::str
   }
 
   ARROW_CHECK_NE(out_flatbuffer, nullptr);
-  out_flatbuffer->assign(reinterpret_cast<const char*>(decompressed_data.data()), raw_len);
+  out_flatbuffer->assign(reinterpret_cast<const char*>(decompressed_data.data()),
+                         raw_len);
 
   return compressed_len + 42;
 }
diff --git a/cpp/src/parquet/metadata3.h b/cpp/src/parquet/metadata3.h
@@ -32,19 +32,20 @@ bool ToFlatbuffer(format::FileMetaData* md, std::string* flatbuf);
 // The flatbuffer in `from` must be valid (such as one retured by `ToFlatbuffer`).
 format::FileMetaData FromFlatbuffer(const format3::FileMetaData* md);
 
-
 // Append/extract the flatbuffer from the footer as a thrift extension:
 // https://github.com/apache/parquet-format/blob/master/BinaryProtocolExtensions.md.
 //
 // `flatbuf` is the flatbuffer representation of the footer metadata.
-// `thrift` is the buffer containing the thrift representation of the footer metadata as its suffix.
+// `thrift` is the buffer containing the thrift representation of the footer metadata as
+// its suffix.
 //
 // Returns the number of bytes added.
 //
 // The extension itself is as follows:
 //
 // +-------------------+------------+--------------------------------------+----------------+---------+--------------------------------+------+
-// | compress(flatbuf) | compressor | crc(compress(flatbuf) .. compressor) | compressed_len | raw_len | crc(compressed_len .. raw_len) | UUID |
+// | compress(flatbuf) | compressor | crc(compress(flatbuf) .. compressor) |
+// compressed_len | raw_len | crc(compressed_len .. raw_len) | UUID |
 // +-------------------+------------+--------------------------------------+----------------+---------+--------------------------------+------+
 //
 // flatbuf: the flatbuffer representation of the footer metadata.
@@ -65,7 +66,7 @@ void AppendFlatbuffer(std::string flatbuffer, std::string* thrift);
 // Returns the size of the flatbuffer if found (and writes to out_flatbuffer),
 // returns 0 if no flatbuffer extension is present, or returns the required
 // buffer size if the input buffer is too small.
-::arrow::Result<int32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf, std::string* out_flatbuffer);
-
-}  // using namespace parquet
+::arrow::Result<uint32_t> ExtractFlatbuffer(std::shared_ptr<Buffer> buf,
+                                            std::string* out_flatbuffer);
 
+}  // namespace parquet
diff --git a/cpp/src/parquet/metadata3_test.cc b/cpp/src/parquet/metadata3_test.cc
diff --git a/cpp/src/parquet/properties.h b/cpp/src/parquet/properties.h
