feat(puffin): add puffin file reader and writer by zhaoxuan1994 · Pull Request #624 · apache/iceberg-cpp

zhaoxuan1994 · 2026-04-23T13:01:21Z

PuffinWriter: in-memory writer that builds complete Puffin files
- Add() writes blobs with optional compression
- Finish() serializes footer with JSON metadata
- Tracks BlobMetadata for all written blobs
PuffinReader: in-memory reader that parses Puffin files
- ReadFileMetadata() parses footer and validates magic bytes
- ReadBlob() reads and decompresses individual blobs
- ReadAll() reads all blobs from metadata
Expose Compress/Decompress as public API in puffin_format.h
Register new sources in CMake and Meson build systems
Add comprehensive tests

Copilot

Pull request overview

Adds an in-memory Puffin file writer/reader to the Iceberg C++ Puffin module, exposes compression helpers as public API, wires new sources into the build, and introduces round-trip + Java-compatibility tests.

Changes:

Introduce PuffinWriter for building Puffin files in-memory (blob writing + footer serialization).
Introduce PuffinReader for parsing Puffin files in-memory (footer parsing + blob reading).
Export Compress/Decompress APIs and register new sources/tests in CMake + Meson.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/iceberg/puffin/puffin_writer.h	Declares the in-memory Puffin writer API.
src/iceberg/puffin/puffin_writer.cc	Implements Puffin file serialization (header, blobs, footer).
src/iceberg/puffin/puffin_reader.h	Declares the in-memory Puffin reader API.
src/iceberg/puffin/puffin_reader.cc	Implements footer parsing, magic validation, and blob reads.
src/iceberg/puffin/puffin_format.h	Exposes `Compress`/`Decompress` as public APIs.
src/iceberg/puffin/puffin_format.cc	Moves flag helpers and keeps compression stubs in the public namespace.
src/iceberg/puffin/meson.build	Installs new public headers for Meson builds.
src/iceberg/meson.build	Adds new Puffin reader/writer sources to the core library build.
src/iceberg/CMakeLists.txt	Adds new Puffin reader/writer sources to the CMake build.
src/iceberg/test/puffin_reader_writer_test.cc	Adds comprehensive writer/reader round-trip and Java-compat tests.
src/iceberg/test/meson.build	Adds the new Puffin reader/writer test to Meson test target.
src/iceberg/test/CMakeLists.txt	Adds the new Puffin reader/writer test to CMake test target.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+/// Parses a Puffin file from an in-memory buffer. Usage:
+///   PuffinReader reader(file_data);
+///   auto metadata = reader.ReadFileMetadata();
+///   auto blob = reader.ReadBlob(metadata->blobs[0]);


kamcheungting-db · 2026-05-04T04:38:52Z

+  auto payload_size = ReadLittleEndian<int32_t>(
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructPayloadSizeOffset);
+
+  // Calculate total footer size and validate
+  int64_t footer_size = PuffinFormat::kFooterStartMagicLength +
+                        static_cast<int64_t>(payload_size) +
+                        PuffinFormat::kFooterStructLength;
+  auto footer_offset = file_size - footer_size;
+  if (footer_offset < 0) {
+    return Invalid("Invalid file: footer size {} exceeds file size {}", footer_size,
+                   file_size);
+  }
+
+  // Validate footer start magic
+  ICEBERG_RETURN_UNEXPECTED(CheckMagic(data_, footer_offset));
+
+  // Check flags for footer compression
+  std::array<uint8_t, 4> flags{};
+  std::memcpy(
+      flags.data(),
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset, 4);
+
+  PuffinCompressionCodec footer_compression = PuffinCompressionCodec::kNone;
+  if (IsFlagSet(flags, PuffinFlag::kFooterPayloadCompressed)) {
+    footer_compression = PuffinFormat::kDefaultFooterCompressionCodec;
+  }
+
+  // Extract footer payload
+  auto payload_offset = footer_offset + PuffinFormat::kFooterStartMagicLength;
+  std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);
+  ICEBERG_ASSIGN_OR_RAISE(auto payload_bytes,
+                          Decompress(footer_compression, payload_span));


payload_size is already validated above:

if (payload_size < 0) { return Invalid("Invalid file: negative payload size {}", payload_size); }

+  if (blob_metadata.offset < 0 ||
+      blob_metadata.offset + blob_metadata.length > file_size) {
+    return Invalid("Invalid blob: offset {} + length {} exceeds file size {}",
+                   blob_metadata.offset, blob_metadata.length, file_size);
+  }
+
+  std::span<const std::byte> raw_data(data_.data() + blob_metadata.offset,
+                                      blob_metadata.length);


- PuffinWriter: in-memory writer that builds complete Puffin files - Add() writes blobs with optional compression - Finish() serializes footer with JSON metadata - Tracks BlobMetadata for all written blobs - PuffinReader: in-memory reader that parses Puffin files - ReadFileMetadata() parses footer and validates magic bytes - ReadBlob() reads and decompresses individual blobs - ReadAll() reads all blobs from metadata - Expose Compress/Decompress as public API in puffin_format.h - Register new sources in CMake and Meson build systems - Add comprehensive tests including Java binary compatibility

kamcheungting-db · 2026-05-04T04:25:07Z

  std::unreachable();
 }

+}  // namespace


is there any reason to move both Result<std::vector<std::byte>> Compress and Result<std::vector<std::byte>> Decompress out of the namespace?

kamcheungting-db

Some minor comments.
Nice code

kamcheungting-db · 2026-05-04T04:32:52Z

+  }
+  auto* begin = reinterpret_cast<const uint8_t*>(data.data() + offset);
+  if (!std::equal(PuffinFormat::kMagicV1.begin(), PuffinFormat::kMagicV1.end(), begin)) {
+    return Invalid("Invalid file: expected magic at offset {}", offset);


please also save the value of incorrect magic number into Invalid for easy debugging.

kamcheungting-db · 2026-05-04T04:33:58Z

+  auto payload_size = ReadLittleEndian<int32_t>(
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructPayloadSizeOffset);
+
+  if (payload_size < 0) {


should payload_size == 0 also be considered as an error.

kamcheungting-db · 2026-05-04T04:35:45Z

+  std::memcpy(
+      flags.data(),
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset, 4);


Suggested change

std::memcpy(

flags.data(),

data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset, 4);

std::memcpy(

flags.data(),

data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset,

4);

kamcheungting-db · 2026-05-04T04:38:52Z

+  auto payload_size = ReadLittleEndian<int32_t>(
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructPayloadSizeOffset);
+
+  // Calculate total footer size and validate
+  int64_t footer_size = PuffinFormat::kFooterStartMagicLength +
+                        static_cast<int64_t>(payload_size) +
+                        PuffinFormat::kFooterStructLength;
+  auto footer_offset = file_size - footer_size;
+  if (footer_offset < 0) {
+    return Invalid("Invalid file: footer size {} exceeds file size {}", footer_size,
+                   file_size);
+  }
+
+  // Validate footer start magic
+  ICEBERG_RETURN_UNEXPECTED(CheckMagic(data_, footer_offset));
+
+  // Check flags for footer compression
+  std::array<uint8_t, 4> flags{};
+  std::memcpy(
+      flags.data(),
+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset, 4);
+
+  PuffinCompressionCodec footer_compression = PuffinCompressionCodec::kNone;
+  if (IsFlagSet(flags, PuffinFlag::kFooterPayloadCompressed)) {
+    footer_compression = PuffinFormat::kDefaultFooterCompressionCodec;
+  }
+
+  // Extract footer payload
+  auto payload_offset = footer_offset + PuffinFormat::kFooterStartMagicLength;
+  std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);
+  ICEBERG_ASSIGN_OR_RAISE(auto payload_bytes,
+                          Decompress(footer_compression, payload_span));


payload_size is already validated above:

if (payload_size < 0) { return Invalid("Invalid file: negative payload size {}", payload_size); }

kamcheungting-db · 2026-05-04T04:42:05Z

+
+  // Extract footer payload
+  auto payload_offset = footer_offset + PuffinFormat::kFooterStartMagicLength;
+  std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);


Suggested change

std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);

const std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);

kamcheungting-db · 2026-05-04T05:01:03Z

+  PuffinCompressionCodec default_codec_;
+  std::vector<std::byte> buffer_;
+  std::vector<BlobMetadata> written_blobs_metadata_;
+  bool header_written_ = false;
+  bool finished_ = false;
+  std::optional<int64_t> footer_size_;


please add one line comment for these varaibles

kamcheungting-db · 2026-05-04T05:02:38Z

+    : default_codec_(default_codec) {}
+
+void PuffinWriter::WriteHeader() {
+  if (header_written_) return;


shouldn't we error out here?
It looks like dangerous to call WriteHeader() multiple time.

Suggested change

if (header_written_) return;

if (header_written_) {

return Invalid("Header has already been written.");

}

kamcheungting-db · 2026-05-04T05:08:52Z

+  };
+
+  auto footer_json = ToJsonString(file_metadata);
+  auto footer_payload = std::span<const std::byte>(


Suggested change

auto footer_payload = std::span<const std::byte>(

const auto footer_payload = std::span<const std::byte>(

kamcheungting-db · 2026-05-04T05:12:13Z

+  buffer_.insert(buffer_.end(), reinterpret_cast<const std::byte*>(magic.data()),
+                 reinterpret_cast<const std::byte*>(magic.data() + magic.size()));


Suggested change

buffer_.insert(buffer_.end(), reinterpret_cast<const std::byte*>(magic.data()),

reinterpret_cast<const std::byte*>(magic.data() + magic.size()));

buffer_.insert(buffer_.end(),

reinterpret_cast<const std::byte*>(magic.data()),

reinterpret_cast<const std::byte*>(magic.data() + magic.size()));

kamcheungting-db · 2026-05-04T05:13:00Z

+  /// \param properties File-level properties to include in the footer.
+  /// \return The complete Puffin file as a byte vector, or an error.
+  Result<std::vector<std::byte>> Finish(
+      std::unordered_map<std::string, std::string> properties = {});


do we really need to copy the whole std::unordered_map<std::string, std::string>

Why do not set the properties while creating the writer?

BTW, it seems that we need to extend our FileIO abstraction to support puffin I/O. It looks inefficient to write all blobs into a buffer like this.

wgtmac

Sorry for the long delay. I have some concerns about its API design which would be inefficient in the real world use case. I believe the main issue arises from the lack of streaming support of FileIO. I will put up a PR to address this.

wgtmac · 2026-05-05T03:30:19Z

+///
+/// Parses a Puffin file from an in-memory buffer. Usage:
+///   PuffinReader reader(file_data);
+///   auto metadata = reader.ReadFileMetadata();


This might not be the recommended approach unless error handling has been added. Otherwise users may silently ignore the error and call metadata.value() unconsciously.

wgtmac · 2026-05-05T03:43:15Z

+class ICEBERG_EXPORT PuffinReader {
+ public:
+  /// \brief Construct a reader from file data.
+  explicit PuffinReader(std::span<const std::byte> data);


The main issue of this ctor is that we lose the capability of seeking into a segment of a puffin file, which is the main use case of v3 deletion vector.

wgtmac · 2026-05-05T03:45:38Z

+      data_.data() + footer_struct_offset + PuffinFormat::kFooterStructFlagsOffset, 4);
+
+  PuffinCompressionCodec footer_compression = PuffinCompressionCodec::kNone;
+  if (IsFlagSet(flags, PuffinFlag::kFooterPayloadCompressed)) {


Please validate that all unknown/reserved footer flag bits are unset. Java's PuffinReader rejects unknown flags, and the spec says reserved bits should be 0; silently ignoring them may cause C++ to accept future or invalid files and interpret the footer incorrectly.

wgtmac · 2026-05-05T08:08:52Z

+
+/// \brief Writer for Puffin files.
+///
+/// Builds a complete Puffin file in memory. Usage:


Same as my comment in the reader, I think we should remove comment like this.

wgtmac · 2026-05-05T08:10:10Z

+
+  /// \brief Add a blob to be written.
+  /// \return The BlobMetadata for the written blob, or an error.
+  Result<BlobMetadata> Add(const Blob& blob);


Do we really need to return a copy of BlobMetadata here? IMO, users usually do not care about it and it pays to do so.

wgtmac · 2026-05-05T08:19:46Z

+ public:
+  /// \brief Construct a writer with the given default compression codec.
+  explicit PuffinWriter(
+      PuffinCompressionCodec default_codec = PuffinCompressionCodec::kNone);


Do we need to introduce a PuffinWriterOptions to hold all possible (but optional) configs in case of expanding the ctor signature in the future?

wgtmac · 2026-05-05T08:20:38Z

+  /// \param properties File-level properties to include in the footer.
+  /// \return The complete Puffin file as a byte vector, or an error.
+  Result<std::vector<std::byte>> Finish(
+      std::unordered_map<std::string, std::string> properties = {});


Why do not set the properties while creating the writer?

wgtmac · 2026-05-05T08:25:01Z

+  /// \param properties File-level properties to include in the footer.
+  /// \return The complete Puffin file as a byte vector, or an error.
+  Result<std::vector<std::byte>> Finish(
+      std::unordered_map<std::string, std::string> properties = {});


BTW, it seems that we need to extend our FileIO abstraction to support puffin I/O. It looks inefficient to write all blobs into a buffer like this.

Copilot AI review requested due to automatic review settings April 23, 2026 13:01

Copilot started reviewing on behalf of zhaoxuan1994 April 23, 2026 13:01 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

zhaoxuan1994 force-pushed the feat/puffin-format-utils branch from 1dd5373 to f8c15ee Compare April 24, 2026 02:13

kamcheungting-db reviewed May 4, 2026

View reviewed changes

wgtmac reviewed May 5, 2026

View reviewed changes

	std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);
	const std::span<const std::byte> payload_span(data_.data() + payload_offset, payload_size);

-  if (header_written_) return;
+  if (header_written_) {
+      return Invalid("Header has already been written.");
+  }

	auto footer_payload = std::span<const std::byte>(
	const auto footer_payload = std::span<const std::byte>(

		buffer_.insert(buffer_.end(), reinterpret_cast<const std::byte*>(magic.data()),
		reinterpret_cast<const std::byte*>(magic.data() + magic.size()));

Conversation

zhaoxuan1994 commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamcheungting-db left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants