feat(puffin): add format constants, utilities, and JSON serialization#603
feat(puffin): add format constants, utilities, and JSON serialization#603zhaoxuan1994 wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
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_formatconstants/utilities (footer flags, little-endian I/O, compression dispatch stubs). - Add
puffin_json_internalJSON serialization/deserialization forBlobMetadataandFileMetadata. - Add
puffin_testand 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.
zhaoxuan1994
force-pushed
the
feat/puffin-format-utils
branch
2 times, most recently
from
18a4dcd to
39d4a91
Compare
| 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 was a problem hiding this comment.
There is very code for this in the roaring bitmap code it should probably standardized in a common place?
There was a problem hiding this comment.
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.
|
|
||
| /// \brief Decompress data using the specified codec. | ||
| ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec, | ||
| std::span<const uint8_t> input); |
There was a problem hiding this comment.
Roaring bitmap is using string_view as collection of bytes, we should probably standardize on one of them.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
|
|
||
| /// \brief Deserialize a FileMetadata from a JSON string. | ||
| ICEBERG_EXPORT Result<FileMetadata> FileMetadataFromJsonString( | ||
| const std::string& json_string); |
There was a problem hiding this comment.
nit: std::string_view?
There was a problem hiding this comment.
seems like tests should be seperated per .cc?
zhaoxuan1994
force-pushed
the
feat/puffin-format-utils
branch
2 times, most recently
from
2ec6aa5 to
abc7aa8
Compare
zhaoxuan1994
force-pushed
the
feat/puffin-format-utils
branch
4 times, most recently
from
7c823ec to
a60eb24
Compare
zhaoxuan1994
force-pushed
the
feat/puffin-format-utils
branch
2 times, most recently
from
4f3ee3b to
3ec8aba
Compare
zhaoxuan1994
force-pushed
the
feat/puffin-format-utils
branch
from
3ec8aba to
e41c4d8
Compare
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