feat(puffin): add basic data structures and constants by zhaoxuan1994 · Pull Request #588 · apache/iceberg-cpp

zhaoxuan1994 · 2026-03-11T08:23:57Z

Add the foundational types for Puffin file format support:

Blob, BlobMetadata, FileMetadata structs
PuffinCompressionCodec enum with codec name conversion
StandardBlobTypes and StandardPuffinProperties constants
ToString functions for all types
24 unit tests covering all public APIs

Copilot

Pull request overview

Adds foundational C++ types and helpers for Iceberg Puffin file support, plus unit tests, and wires them into the build.

Changes:

Introduces core Puffin data structures (Blob, BlobMetadata, FileMetadata) and standard constants (StandardBlobTypes, StandardPuffinProperties).
Adds PuffinCompressionCodec with name conversion/parsing helpers and ToString() functions.
Adds a dedicated puffin_test suite and integrates Puffin sources into the Iceberg library build.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/iceberg/puffin/blob.h	Defines `Blob` structure for uncompressed blob payload + metadata.
src/iceberg/puffin/blob.cc	Implements `ToString(const Blob&)`.
src/iceberg/puffin/blob_metadata.h	Defines `BlobMetadata` footer metadata structure.
src/iceberg/puffin/blob_metadata.cc	Implements `ToString(const BlobMetadata&)`.
src/iceberg/puffin/file_metadata.h	Defines `FileMetadata` footer metadata structure.
src/iceberg/puffin/file_metadata.cc	Implements `ToString(const FileMetadata&)`.
src/iceberg/puffin/puffin_compression_codec.h	Declares codec enum + name conversion APIs.
src/iceberg/puffin/puffin_compression_codec.cc	Implements codec name conversion/parsing + `ToString`.
src/iceberg/puffin/types.h	Adds standard Puffin blob type/property string constants.
src/iceberg/puffin/CMakeLists.txt	Installs Puffin headers via CMake.
src/iceberg/CMakeLists.txt	Adds Puffin sources to `ICEBERG_SOURCES` and adds `puffin` subdirectory.
src/iceberg/test/puffin_test.cc	Adds unit tests covering the new Puffin public APIs.
src/iceberg/test/CMakeLists.txt	Registers `puffin_test` in the CMake test suite.

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

You can also share your feedback on Copilot code review. Take the survey.

src/iceberg/puffin/puffin_compression_codec.cc

src/iceberg/CMakeLists.txt

src/iceberg/test/CMakeLists.txt

src/iceberg/puffin/blob.h

evindj

Overall looks good just curious about the choice of using string literals instead of enums in some place.

evindj · 2026-03-13T03:19:06Z

src/iceberg/puffin/types.h

+/// \brief Standard blob types defined by the Iceberg specification.
+struct StandardBlobTypes {
+  /// A serialized form of a "compact" Theta sketch produced by the
+  /// Apache DataSketches library.
+  static constexpr std::string_view kApacheDatasketchesThetaV1 =
+      "apache-datasketches-theta-v1";
+
+  /// A serialized deletion vector according to the Iceberg spec.
+  static constexpr std::string_view kDeletionVectorV1 = "deletion-vector-v1";
+};


Should we do enum + toString ?

Per the Puffin spec, type is a "JSON string" field. The spec lists known blob types under "Blob types" but doesn't restrict the field to only those values — new types can be added as the spec evolves. Both the Java and Rust implementations use String for this field rather than an enum. StandardBlobTypes provides well-known constants to avoid typos while keeping the field open for future extensibility.

evindj · 2026-03-13T03:21:21Z

src/iceberg/puffin/blob.cc

+                 blob.input_fields);
+  std::format_to(std::back_inserter(repr), "snapshotId={},sequenceNumber={},",
+                 blob.snapshot_id, blob.sequence_number);
+  std::format_to(std::back_inserter(repr), "dataSize={}", blob.data.size());


Nit: may be add a checksum if available for fast comparison(I do understand checksum is not part of the puffin spec).

That's an interesting idea! Currently Blob doesn't carry a checksum field (the Puffin spec doesn't include one either), so there's nothing available to display here. For equality comparison, Blob already has operator==. If we want to add checksum support down the road, it would probably be a separate discussion about the struct design itself — happy to explore that if there's a use case!

evindj · 2026-03-13T03:23:51Z

src/iceberg/puffin/blob_metadata.h

+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+  /// Type of the blob. See StandardBlobTypes for known types.
+  std::string type;


It is weird that the type is declared as string. it makes it harder to understand what type of blobs are accepted but harder to catch typos.

Same reasoning as StandardBlobTypes — the spec requires this to be an open string.

evindj · 2026-03-13T03:25:16Z

src/iceberg/puffin/blob_metadata.h

+  int64_t sequence_number;
+  /// Offset in the file where the blob data starts.
+  int64_t offset;
+  /// Length of the blob data in the file (after compression, if compressed).


How to check if the blob is compressed, Is it if the compression_codec is set?

Yes, exactly. If compression_codec has a value, the blob is compressed; if it's std::nullopt, it's uncompressed. I'll improve the comment to make this clearer.

evindj · 2026-03-13T03:25:39Z

src/iceberg/puffin/blob_metadata.h

+  /// Length of the blob data in the file (after compression, if compressed).
+  int64_t length;
+  /// Compression codec name, or std::nullopt if uncompressed.
+  std::optional<std::string> compression_codec;


ditto same pattern string or enum

BlobMetadata is a direct mapping of the Puffin footer JSON, where compression-codec is a JSON string (or absent). The Java implementation also uses @nullable String here. The conversion from string to PuffinCompressionCodec enum happens at a higher level via PuffinCompressionCodecFromName(), which returns Result<> to properly handle unknown codecs. This layering keeps the metadata faithful to the on-disk format and provides forward compatibility — if a future spec version adds a new codec, we can still deserialize the footer successfully and fail gracefully at decompression time rather than at parsing time.

Should we remove std::optional here since an empty string is sufficient to represent the unset state.

evindj · 2026-03-13T03:40:11Z

src/iceberg/puffin/puffin_compression_codec.cc

+    case PuffinCompressionCodec::kZstd:
+      return kZstdCodecName;
+  }
+  std::unreachable();


consider returning Result<> so we can avoid throwing an exception here

std::unreachable() here isn't actually throwing — it's a C++23 compile-time hint that this code path is impossible, since the switch already covers all enum values. This is a common pattern in the codebase (e.g., transform.cc, type.cc, expression.cc, etc.). Returning Result<> would require callers to handle an error that can't happen in practice. That said, PuffinCompressionCodecFromName does return Result<> since its string input can genuinely be invalid — so the two functions intentionally use different strategies based on their input types.

Add the foundational types for Puffin file format support: - Blob, BlobMetadata, FileMetadata structs - PuffinCompressionCodec enum with codec name conversion - StandardBlobTypes and StandardPuffinProperties constants - ToString functions for all types - 24 unit tests covering all public APIs

wgtmac

Sorry for the long delay! I have left some general comments. Let me know what you think.

wgtmac · 2026-03-19T15:35:09Z

src/iceberg/puffin/blob.h

+/// \brief A blob to be written to a Puffin file.
+///
+/// This represents the uncompressed blob data along with its metadata.
+/// The actual compression is handled during writing.


Suggested change

/// \brief A blob to be written to a Puffin file.

///

/// This represents the uncompressed blob data along with its metadata.

/// The actual compression is handled during writing.

/// \brief A blob in a Puffin file.

Let's be concise and accurate. We don't need to mention either write or compress here since this is just a blob entry.

wgtmac · 2026-03-19T15:36:09Z

src/iceberg/puffin/blob.h

+/// This represents the uncompressed blob data along with its metadata.
+/// The actual compression is handled during writing.
+struct ICEBERG_EXPORT Blob {
+  /// Type of the blob. See StandardBlobTypes for known types.


Suggested change

/// Type of the blob. See StandardBlobTypes for known types.

/// \brief Type of the blob. See StandardBlobTypes for known types.

Let's consistently use the doxygen style comment

wgtmac · 2026-03-19T15:43:39Z

src/iceberg/puffin/blob_metadata.h

+  /// Length of the blob data in the file. If compression_codec is set, this is
+  /// the compressed size; otherwise it is the raw size.


Suggested change

/// Length of the blob data in the file. If compression_codec is set, this is

/// the compressed size; otherwise it is the raw size.

/// \brief Length of the blob data in the file.

IMO this is obvious.

wgtmac · 2026-03-19T15:44:29Z

src/iceberg/puffin/blob_metadata.h

+  /// Length of the blob data in the file (after compression, if compressed).
+  int64_t length;
+  /// Compression codec name, or std::nullopt if uncompressed.
+  std::optional<std::string> compression_codec;


Should we remove std::optional here since an empty string is sufficient to represent the unset state.

wgtmac · 2026-03-19T15:46:58Z

src/iceberg/puffin/file_metadata.h

+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a Puffin file.


I'd recommend putting definitions of Blob, BlobMetadata and other puffin related classes to this file. We don't need to use a separate file for each class.

wgtmac · 2026-03-19T15:48:21Z

src/iceberg/puffin/puffin_compression_codec.cc

+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+  switch (codec) {
+    case PuffinCompressionCodec::kNone:
+      return std::nullopt;


Why not returning an empty string? Then you don't need to wrap the return type with optional.

wgtmac · 2026-03-19T15:48:37Z

src/iceberg/puffin/puffin_compression_codec.cc

+}
+
+Result<PuffinCompressionCodec> PuffinCompressionCodecFromName(
+    std::optional<std::string_view> codec_name) {


Same question as above.

wgtmac · 2026-03-19T15:58:04Z

src/iceberg/test/puffin_test.cc

+// Blob Tests
+// ============================================================================
+
+TEST(BlobTest, Equality) {


TBH, I think test cases like this are unnecessary.

Copilot AI review requested due to automatic review settings March 11, 2026 08:23

Copilot started reviewing on behalf of zhaoxuan1994 March 11, 2026 08:24 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

src/iceberg/puffin/puffin_compression_codec.cc Show resolved Hide resolved

src/iceberg/CMakeLists.txt Show resolved Hide resolved

src/iceberg/test/CMakeLists.txt Show resolved Hide resolved

src/iceberg/puffin/blob.h Outdated Show resolved Hide resolved

zhaoxuan1994 force-pushed the feat/puffin-basic-types branch 2 times, most recently from 8a7ed69 to 7e5818d Compare March 11, 2026 09:29

evindj reviewed Mar 13, 2026

View reviewed changes

zhaoxuan1994 force-pushed the feat/puffin-basic-types branch from 7e5818d to 0721621 Compare March 16, 2026 04:09

wgtmac requested changes Mar 19, 2026

View reviewed changes

	/// Type of the blob. See StandardBlobTypes for known types.
	/// \brief Type of the blob. See StandardBlobTypes for known types.

		/// Length of the blob data in the file. If compression_codec is set, this is
		/// the compressed size; otherwise it is the raw size.

	/// Length of the blob data in the file. If compression_codec is set, this is
	/// the compressed size; otherwise it is the raw size.
	/// \brief Length of the blob data in the file.


		namespace iceberg::puffin {

		/// \brief Metadata about a Puffin file.

Conversation

zhaoxuan1994 commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evindj 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!

zhaoxuan1994 Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zhaoxuan1994 Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

zhaoxuan1994 Mar 13, 2026 •

edited

Loading

zhaoxuan1994 Mar 13, 2026 •

edited

Loading