feat(puffin): add format constants, utilities, and JSON serialization by zhaoxuan1994 · Pull Request #603 · apache/iceberg-cpp

zhaoxuan1994 · 2026-03-24T07:06:29Z

Second PR in the Puffin series (following #588).

puffin_format.h/.cc: format constants, footer flag operations, little-endian I/O, compress/decompress dispatch (LZ4/Zstd stubbed as NotSupported)
puffin_json_internal.h/.cc: JSON serialization for BlobMetadata and FileMetadata

Copilot

Pull request overview

Adds core Puffin file-format helpers and JSON (de)serialization support for Puffin metadata, plus unit tests and build-system wiring to compile/run the new components.

Changes:

Introduce puffin_format constants/utilities (footer flags, little-endian I/O, compression dispatch stubs).
Add puffin_json_internal JSON serialization/deserialization for BlobMetadata and FileMetadata.
Add puffin_test and register it in both CMake and Meson test targets.

Reviewed changes

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

Show a summary per file

File	Description
`src/iceberg/puffin/puffin_format.h`	Declares Puffin format constants and helper APIs (flags, endian I/O, compress/decompress).
`src/iceberg/puffin/puffin_format.cc`	Implements flags/endian helpers and compression dispatch stubs.
`src/iceberg/puffin/puffin_json_internal.h`	Declares JSON (de)serialization APIs for Puffin metadata.
`src/iceberg/puffin/puffin_json_internal.cc`	Implements JSON (de)serialization and string parsing for `FileMetadata`.
`src/iceberg/test/puffin_test.cc`	Adds unit tests for endian/flags/compression stubs and JSON round-trips.
`src/iceberg/test/CMakeLists.txt`	Registers `puffin_test` in CMake-based test build.
`src/iceberg/test/meson.build`	Registers `puffin_test` in Meson-based test build.
`src/iceberg/meson.build`	Adds new Puffin `.cc` sources to the main Meson library build.
`src/iceberg/CMakeLists.txt`	Adds new Puffin `.cc` sources to the main CMake library build.
`src/iceberg/puffin/meson.build`	Updates Puffin header installation list.

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

src/iceberg/puffin/puffin_format.cc

src/iceberg/puffin/puffin_json_internal.cc

src/iceberg/puffin/meson.build

Copilot · 2026-03-24T07:11:28Z

src/iceberg/puffin/puffin_format.h

+
+  /// Default compression codec for footer payload.
+  static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+      PuffinCompressionCodec::kLz4;


PuffinFormat::kFooterCompressionCodec is set to kLz4, but Compress/Decompress currently return NotSupported for LZ4. This constant is likely to be used as the default when writing/reading footer payloads, and will cause immediate failures once that code lands. Consider defaulting to kNone until LZ4 support is implemented, or clearly documenting that the default codec is not yet supported and must not be used for writing.

Suggested change

PuffinCompressionCodec::kLz4;

PuffinCompressionCodec::kNone;

Keeping kLz4 as it reflects the Puffin spec's default footer compression codec, consistent with the Java implementation. This constant is a spec-level declaration — actual usage in the writer will check the Result from Compress(), so unsupported codecs surface as explicit errors.

I think this is actually a real compatibility concern where java puffin readers would likely break if a compressed footer was ever properly written. Java has never implemented lz4 compression: https://github.com/apache/iceberg/blob/8a51a685894eace51474f512b46a187566f27202/core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java#L112 which IIUC means anything written with a compressed footer would not be readable from Java. If you agree with this assessment it might be misreading the code but might be worth a discussion on the mailing list.

Looking more closely it looks like java would just throw unsupported codec for compressed footers

I agree the spec calls for lz4, so there is maybe nothing to be done here

emkornfield · 2026-03-25T20:06:08Z

src/iceberg/puffin/puffin_format.h

+ICEBERG_EXPORT void WriteInt32LittleEndian(int32_t value, std::span<uint8_t, 4> output);
+
+/// \brief Read a 32-bit integer from a fixed-size span in little-endian format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t, 4> input);


There is very code for this in the roaring bitmap code it should probably standardized in a common place?

Done. Added WriteLittleEndian/ReadLittleEndian templates to endian.h as public API. Updated both puffin_format.cc and roaring_position_bitmap.cc to use them, removing the duplicated local helpers.

emkornfield · 2026-03-25T20:07:29Z

src/iceberg/puffin/puffin_format.h

+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+                                                       std::span<const uint8_t> input);


Roaring bitmap is using string_view as collection of bytes, we should probably standardize on one of them.

I used span here since it feels more natural for binary data. But I can see the argument for string_view given that CRoaring already uses std::string for serialization. Do you have a preference on which one the project should standardize on?

I don't. Another things to decide is if we are using C++17 or above if std::byte should be used instead. This is probably something that should be discussed on the mailing list I would guess, or at least get @wgtmac and other committers opinion on.

We're on C++23 so std::byte is definitely available. Together with my previous comment for a generic codec interface/implementation, we can use std::span<const std::byte> as input and std::vector<std::byte> as output but with overloads that accepting std::string_view and returning std::string, etc.

@wgtmac I guess my main concern here would be to try to maintain internal consistency within all the code in the project. For consuming use-cases the overloads might be worth it. I wonder if it would be too premature to consider having a Buffer object?

I'm hesitant to introduce a Buffer (and corresponding MemoryPool) at this moment. They are only used as ephemeral input/output buffers for CompressionCodec and FileIO.

emkornfield · 2026-03-25T20:15:40Z

src/iceberg/puffin/puffin_json_internal.h

+
+/// \brief Deserialize a FileMetadata from a JSON string.
+ICEBERG_EXPORT Result<FileMetadata> FileMetadataFromJsonString(
+    const std::string& json_string);


nit: std::string_view?

emkornfield · 2026-03-25T20:19:08Z

src/iceberg/test/puffin_json_test.cc

seems like tests should be seperated per .cc?

src/iceberg/CMakeLists.txt

src/iceberg/puffin/puffin_format.h

src/iceberg/test/CMakeLists.txt

src/iceberg/puffin/puffin_format.h

src/iceberg/puffin/puffin_format.cc

src/iceberg/test/puffin_format_test.cc

src/iceberg/test/puffin_json_test.cc

wgtmac · 2026-04-01T15:20:49Z

src/iceberg/util/endian.h

  }
 }

+/// \brief Write a value in little-endian format to a buffer.


It would be good to add a comment to say that it is caller's responsibility to guarantee that output has sufficient length, otherwise it is a UB.

wgtmac · 2026-04-01T15:20:56Z

src/iceberg/util/endian.h

+
+/// \brief Read a value in little-endian format from a buffer.
+template <EndianConvertible T>
+T ReadLittleEndian(const void* input) {


wgtmac · 2026-04-01T15:24:54Z

src/iceberg/puffin/file_metadata.h


 ICEBERG_EXPORT std::string ToString(const BlobMetadata& blob_metadata);

+inline std::ostream& operator<<(std::ostream& os, const BlobMetadata& b) {


We don't have any precedence to add this. It is preferable to use std::format wherever possible.

wgtmac · 2026-04-01T15:26:18Z

src/iceberg/puffin/json_serde_internal.h

+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"


Can we remove this and add a iceberg/puffin/type_fwd.h for forward declarations?

wgtmac · 2026-04-01T15:39:43Z

src/iceberg/puffin/puffin_format.h

+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT Result<int32_t> ReadInt32LittleEndian(std::span<const uint8_t> data,


Should we move this to endian.h? If not, I'd be inclined to remove this for now and then add it back as an internal function in the source file where it is called.

wgtmac · 2026-04-01T15:41:01Z

src/iceberg/puffin/json_serde_internal.h

+ICEBERG_EXPORT Result<FileMetadata> FileMetadataFromJson(const nlohmann::json& json);
+
+/// \brief Serialize a FileMetadata to a JSON string.
+ICEBERG_EXPORT std::string ToJsonString(const FileMetadata& file_metadata,


Do we really need to specialize this?

wgtmac · 2026-04-01T15:43:25Z

src/iceberg/test/puffin_format_test.cc

+namespace iceberg::puffin {
+
+TEST(PuffinFormatTest, ByteOrderRoundTrip) {
+  std::array<uint8_t, 4> buf{};


This test seems unrelated to puffin. Move it to endian_test.cc?

wgtmac · 2026-04-01T15:44:22Z

src/iceberg/test/puffin_json_test.cc

+INSTANTIATE_TEST_SUITE_P(
+    PuffinJson, InvalidBlobMetadataJsonTest,
+    ::testing::Values(
+        InvalidBlobMetadataJsonParam{.name = "MissingType",


Add a MissingSequenceNumber test case for completeness.

Copilot AI review requested due to automatic review settings March 24, 2026 07:06

Copilot started reviewing on behalf of zhaoxuan1994 March 24, 2026 07:06 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

zhaoxuan1994 force-pushed the feat/puffin-format-utils branch 2 times, most recently from 18a4dcd to 39d4a91 Compare March 24, 2026 07:33

emkornfield reviewed Mar 25, 2026

View reviewed changes

zhaoxuan1994 force-pushed the feat/puffin-format-utils branch 2 times, most recently from 2ec6aa5 to abc7aa8 Compare March 26, 2026 07:23

wgtmac requested changes Mar 27, 2026

View reviewed changes

zhaoxuan1994 force-pushed the feat/puffin-format-utils branch 3 times, most recently from 6163111 to 7c823ec Compare March 30, 2026 08:49

feat(puffin): add format constants, utilities, and JSON serialization

a60eb24

zhaoxuan1994 force-pushed the feat/puffin-format-utils branch from 7c823ec to a60eb24 Compare March 30, 2026 09:40

zhaoxuan1994 requested a review from wgtmac March 30, 2026 11:40

wgtmac reviewed Apr 1, 2026

View reviewed changes


		ICEBERG_EXPORT std::string ToString(const BlobMetadata& blob_metadata);

		inline std::ostream& operator<<(std::ostream& os, const BlobMetadata& b) {

Conversation

zhaoxuan1994 commented Mar 24, 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!

Copilot AI Mar 24, 2026

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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