You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zhaoxuan1994 (via GitHub)" <gi...@apache.org> on 2026/03/24 07:06:33 UTC
[PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
zhaoxuan1994 opened a new pull request, #603:
URL: https://github.com/apache/iceberg-cpp/pull/603
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2993475862
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review Comment:
I used span<const uint8_t> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2993042218
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
Review Comment:
Looking more closely it looks like java would just throw unsupported codec for compressed footers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2979497079
##########
src/iceberg/puffin/puffin_json_internal.cc:
##########
@@ -0,0 +1,127 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_json_internal.h"
+
+#include <nlohmann/json.hpp>
+
+#include "iceberg/util/json_util_internal.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kBlobs = "blobs";
+constexpr std::string_view kProperties = "properties";
+constexpr std::string_view kType = "type";
+constexpr std::string_view kFields = "fields";
+constexpr std::string_view kSnapshotId = "snapshot-id";
+constexpr std::string_view kSequenceNumber = "sequence-number";
+constexpr std::string_view kOffset = "offset";
+constexpr std::string_view kLength = "length";
+constexpr std::string_view kCompressionCodec = "compression-codec";
+} // namespace
+
+nlohmann::json ToJson(const BlobMetadata& blob_metadata) {
+ nlohmann::json json;
+ json[kType] = blob_metadata.type;
+ json[kFields] = blob_metadata.input_fields;
+ json[kSnapshotId] = blob_metadata.snapshot_id;
+ json[kSequenceNumber] = blob_metadata.sequence_number;
+ json[kOffset] = blob_metadata.offset;
+ json[kLength] = blob_metadata.length;
+
+ SetOptionalStringField(json, kCompressionCodec, blob_metadata.compression_codec);
+ SetContainerField(json, kProperties, blob_metadata.properties);
+
+ return json;
+}
+
+Result<BlobMetadata> BlobMetadataFromJson(const nlohmann::json& json) {
+ BlobMetadata blob_metadata;
+
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.type, GetJsonValue<std::string>(json, kType));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.input_fields,
+ GetJsonValue<std::vector<int32_t>>(json, kFields));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.snapshot_id,
+ GetJsonValue<int64_t>(json, kSnapshotId));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.sequence_number,
+ GetJsonValue<int64_t>(json, kSequenceNumber));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.offset, GetJsonValue<int64_t>(json, kOffset));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.length, GetJsonValue<int64_t>(json, kLength));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.compression_codec,
+ GetJsonValueOrDefault<std::string>(json, kCompressionCodec));
+ ICEBERG_ASSIGN_OR_RAISE(blob_metadata.properties,
+ FromJsonMap<std::string>(json, kProperties));
+
+ return blob_metadata;
+}
+
+nlohmann::json ToJson(const FileMetadata& file_metadata) {
+ nlohmann::json json;
+
+ nlohmann::json blobs_json = nlohmann::json::array();
+ for (const auto& blob : file_metadata.blobs) {
+ blobs_json.push_back(ToJson(blob));
+ }
+ json[kBlobs] = std::move(blobs_json);
+
+ SetContainerField(json, kProperties, file_metadata.properties);
+
+ return json;
+}
+
+Result<FileMetadata> FileMetadataFromJson(const nlohmann::json& json) {
+ FileMetadata file_metadata;
+
+ ICEBERG_ASSIGN_OR_RAISE(auto blobs_json, GetJsonValue<nlohmann::json>(json, kBlobs));
+ if (!blobs_json.is_array()) {
+ return InvalidArgument("Cannot parse blobs from non-array: {}",
+ SafeDumpJson(blobs_json));
Review Comment:
`FileMetadataFromJson` returns `InvalidArgument` when the `blobs` field is present but not an array. For JSON deserialization errors elsewhere in the codebase, `JsonParseError` is used (e.g., `json_serde.cc` and `json_util_internal.h`), which makes error kinds consistent for callers that route parse errors differently from argument validation. Consider returning `JsonParseError` here as well.
```suggestion
return JsonParseError("Cannot parse blobs from non-array: {}",
SafeDumpJson(blobs_json));
```
##########
src/iceberg/puffin/meson.build:
##########
@@ -15,4 +15,4 @@
# specific language governing permissions and limitations
# under the License.
-install_headers(['file_metadata.h'], subdir: 'iceberg/puffin')
+install_headers(['file_metadata.h', 'puffin_format.h', 'puffin_json_internal.h'], subdir: 'iceberg/puffin')
Review Comment:
`puffin_json_internal.h` is being installed as a public header even though it is explicitly marked "internal" (and other `*_internal.h` headers in this repo are not installed). Installing it expands the supported public API surface and also exposes a dependency on `nlohmann::json` in the public headers. Consider either (a) removing it from `install_headers` and keeping it internal-only, or (b) renaming/promoting it to a stable public header (and include `nlohmann/json.hpp` instead of only the fwd header if it remains in the API).
```suggestion
install_headers(['file_metadata.h', 'puffin_format.h'], subdir: 'iceberg/puffin')
```
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
Review Comment:
`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.
```suggestion
PuffinCompressionCodec::kNone;
```
##########
src/iceberg/puffin/puffin_format.cc:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_format.h"
+
+#include <cstring>
+#include <utility>
+
+#include "iceberg/util/endian.h"
+
+namespace iceberg::puffin {
+
+namespace {
+
+constexpr std::pair<int, int> GetFlagPosition(PuffinFlag flag) {
+ switch (flag) {
+ case PuffinFlag::kFooterPayloadCompressed:
+ return {0, 0};
+ }
+ std::unreachable();
+}
+
+} // namespace
+
+bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag) {
+ auto [byte_num, bit_num] = GetFlagPosition(flag);
+ return (flags[byte_num] & (1 << bit_num)) != 0;
+}
+
+void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag) {
+ auto [byte_num, bit_num] = GetFlagPosition(flag);
+ flags[byte_num] |= (1 << bit_num);
+}
+
+void WriteInt32LittleEndian(int32_t value, std::span<uint8_t, 4> output) {
+ auto le_value = ToLittleEndian(value);
+ std::memcpy(output.data(), &le_value, sizeof(le_value));
+}
+
+int32_t ReadInt32LittleEndian(std::span<const uint8_t, 4> input) {
+ int32_t value;
+ std::memcpy(&value, input.data(), sizeof(value));
+ return FromLittleEndian(value);
+}
+
+int32_t ReadInt32LittleEndian(std::span<const uint8_t> data, int32_t offset) {
Review Comment:
`ReadInt32LittleEndian(std::span<const uint8_t> data, int32_t offset)` constructs a fixed-size span from `data.data() + offset` without validating `offset` (negative values or `offset + 4 > data.size()`), which can lead to out-of-bounds reads/UB when parsing untrusted buffers. Since this API is newly introduced, consider changing it to return `Result<int32_t>` (or removing the overload) and performing bounds checks (or at least an `ICEBERG_DCHECK`) before forming the subspan.
```suggestion
int32_t ReadInt32LittleEndian(std::span<const uint8_t> data, int32_t offset) {
ICEBERG_DCHECK(offset >= 0);
ICEBERG_DCHECK(static_cast<size_t>(offset) + 4 <= data.size());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2990758363
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review Comment:
Roaring bitmap is using string_view as collection of bytes, we should probably standardize on one of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2993048041
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
Review Comment:
I agree the spec calls for lz4, so there is maybe nothing to be done here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2992927682
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2992962336
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2995185515
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review Comment:
I don't. Another things to decide is if we are using C++17 or above if [std::byte](https://en.cppreference.com/w/cpp/types/byte.html) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2992958994
##########
src/iceberg/test/puffin_json_test.cc:
##########
Review Comment:
Done
##########
src/iceberg/puffin/puffin_json_internal.h:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_json_internal.h
+/// JSON serialization/deserialization for Puffin file metadata.
+
+#include <string>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Serialize a BlobMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const BlobMetadata& blob_metadata);
+
+/// \brief Deserialize a BlobMetadata from JSON.
+ICEBERG_EXPORT Result<BlobMetadata> BlobMetadataFromJson(const nlohmann::json& json);
+
+/// \brief Serialize a FileMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const FileMetadata& file_metadata);
+
+/// \brief Deserialize a FileMetadata from JSON.
+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,
+ bool pretty = false);
+
+/// \brief Deserialize a FileMetadata from a JSON string.
+ICEBERG_EXPORT Result<FileMetadata> FileMetadataFromJsonString(
+ const std::string& json_string);
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r3022851060
##########
src/iceberg/puffin/json_serde_internal.h:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/json_serde_internal.h
+/// JSON serialization/deserialization for Puffin file metadata.
+
+#include <string>
+#include <string_view>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
Review Comment:
Can we remove this and add a `iceberg/puffin/type_fwd.h` for forward declarations?
##########
src/iceberg/util/endian.h:
##########
@@ -94,4 +95,19 @@ constexpr T FromBigEndian(T value) {
}
}
+/// \brief Write a value in little-endian format to a buffer.
+template <EndianConvertible T>
+void WriteLittleEndian(T value, void* output) {
+ auto le = ToLittleEndian(value);
+ std::memcpy(output, &le, sizeof(le));
+}
+
+/// \brief Read a value in little-endian format from a buffer.
+template <EndianConvertible T>
+T ReadLittleEndian(const void* input) {
Review Comment:
ditto
##########
src/iceberg/puffin/file_metadata.h:
##########
@@ -107,6 +108,10 @@ struct ICEBERG_EXPORT BlobMetadata {
ICEBERG_EXPORT std::string ToString(const BlobMetadata& blob_metadata);
+inline std::ostream& operator<<(std::ostream& os, const BlobMetadata& b) {
Review Comment:
We don't have any precedence to add this. It is preferable to use std::format wherever possible.
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagicV1 = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kDefaultFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \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,
Review Comment:
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.
##########
src/iceberg/puffin/json_serde_internal.h:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/json_serde_internal.h
+/// JSON serialization/deserialization for Puffin file metadata.
+
+#include <string>
+#include <string_view>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Serialize a BlobMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const BlobMetadata& blob_metadata);
+
+/// \brief Deserialize a BlobMetadata from JSON.
+ICEBERG_EXPORT Result<BlobMetadata> BlobMetadataFromJson(const nlohmann::json& json);
+
+/// \brief Serialize a FileMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const FileMetadata& file_metadata);
+
+/// \brief Deserialize a FileMetadata from JSON.
+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,
Review Comment:
Do we really need to specialize this?
##########
src/iceberg/test/puffin_format_test.cc:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_format.h"
+
+#include <array>
+#include <cstdint>
+
+#include <gtest/gtest.h>
+
+#include "iceberg/util/endian.h"
+
+namespace iceberg::puffin {
+
+TEST(PuffinFormatTest, ByteOrderRoundTrip) {
+ std::array<uint8_t, 4> buf{};
Review Comment:
This test seems unrelated to puffin. Move it to `endian_test.cc`?
##########
src/iceberg/test/puffin_json_test.cc:
##########
@@ -0,0 +1,193 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <string>
+
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/puffin/json_serde_internal.h"
+#include "iceberg/test/matchers.h"
+
+namespace iceberg::puffin {
+
+// ==================== BlobMetadata Parameterized Tests ====================
+
+struct BlobMetadataJsonParam {
+ std::string name;
+ BlobMetadata blob;
+ std::string expected_json;
+};
+
+class BlobMetadataJsonTest : public ::testing::TestWithParam<BlobMetadataJsonParam> {};
+
+TEST_P(BlobMetadataJsonTest, RoundTrip) {
+ const auto& param = GetParam();
+ auto expected = nlohmann::json::parse(param.expected_json);
+
+ EXPECT_EQ(ToJson(param.blob), expected);
+ EXPECT_THAT(BlobMetadataFromJson(expected), HasValue(::testing::Eq(param.blob)));
+}
+
+INSTANTIATE_TEST_SUITE_P(PuffinJson, BlobMetadataJsonTest,
+ ::testing::Values(
+ BlobMetadataJsonParam{
+ .name = "AllFields",
+ .blob = {.type = "apache-datasketches-theta-v1",
+ .input_fields = {1, 2},
+ .snapshot_id = 12345,
+ .sequence_number = 67,
+ .offset = 100,
+ .length = 200,
+ .compression_codec = "zstd",
+ .properties = {{"key", "value"}}},
+ .expected_json = R"({
+ "type": "apache-datasketches-theta-v1",
+ "fields": [1, 2],
+ "snapshot-id": 12345,
+ "sequence-number": 67,
+ "offset": 100,
+ "length": 200,
+ "compression-codec": "zstd",
+ "properties": {"key": "value"}
+ })"},
+ BlobMetadataJsonParam{.name = "MinimalFields",
+ .blob = {.type = "test-type",
+ .input_fields = {1},
+ .snapshot_id = 100,
+ .sequence_number = 1,
+ .offset = 0,
+ .length = 50},
+ .expected_json = R"({
+ "type": "test-type",
+ "fields": [1],
+ "snapshot-id": 100,
+ "sequence-number": 1,
+ "offset": 0,
+ "length": 50
+ })"}),
+ [](const ::testing::TestParamInfo<BlobMetadataJsonParam>& info) {
+ return info.param.name;
+ });
+
+// ==================== BlobMetadata Invalid JSON Tests ====================
+
+struct InvalidBlobMetadataJsonParam {
+ std::string name;
+ std::string json;
+};
+
+class InvalidBlobMetadataJsonTest
+ : public ::testing::TestWithParam<InvalidBlobMetadataJsonParam> {};
+
+TEST_P(InvalidBlobMetadataJsonTest, DeserializeFails) {
+ auto json = nlohmann::json::parse(GetParam().json);
+ EXPECT_THAT(BlobMetadataFromJson(json), IsError(ErrorKind::kJsonParseError));
+}
+
+INSTANTIATE_TEST_SUITE_P(
+ PuffinJson, InvalidBlobMetadataJsonTest,
+ ::testing::Values(
+ InvalidBlobMetadataJsonParam{.name = "MissingType",
Review Comment:
Add a MissingSequenceNumber test case for completeness.
##########
src/iceberg/util/endian.h:
##########
@@ -94,4 +95,19 @@ constexpr T FromBigEndian(T value) {
}
}
+/// \brief Write a value in little-endian format to a buffer.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2993034830
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2998667206
##########
src/iceberg/CMakeLists.txt:
##########
@@ -64,6 +64,8 @@ set(ICEBERG_SOURCES
partition_spec.cc
partition_summary.cc
puffin/file_metadata.cc
+ puffin/puffin_format.cc
+ puffin/puffin_json_internal.cc
Review Comment:
Let's rename to `puffin/json_serde_internal.h` and `puffin/json_serde.cc` respectively for consistency.
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
Review Comment:
```suggestion
static constexpr PuffinCompressionCodec kDefaultFooterCompressionCodec =
```
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -125,6 +125,10 @@ add_iceberg_test(util_test
add_iceberg_test(roaring_test SOURCES roaring_test.cc)
+add_iceberg_test(puffin_format_test SOURCES puffin_format_test.cc)
+
+add_iceberg_test(puffin_json_test SOURCES puffin_json_test.cc)
Review Comment:
Can we merge them into a single puffin_test executable?
##########
src/iceberg/test/puffin_format_test.cc:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_format.h"
+
+#include <array>
+#include <cstdint>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+namespace iceberg::puffin {
+
+TEST(PuffinFormatTest, ByteOrderRoundTrip) {
+ std::array<uint8_t, 4> buf{};
+ WriteInt32LittleEndian(0x12345678, buf);
Review Comment:
Should these functions be moved to endian.h as they are not puffin specific?
##########
src/iceberg/puffin/puffin_format.cc:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_format.h"
+
+#include <utility>
+
+#include "iceberg/util/endian.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::puffin {
+
+namespace {
+
+constexpr std::pair<int, int> GetFlagPosition(PuffinFlag flag) {
+ switch (flag) {
+ case PuffinFlag::kFooterPayloadCompressed:
+ return {0, 0};
+ }
+ std::unreachable();
+}
+
+} // namespace
+
+bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag) {
+ auto [byte_num, bit_num] = GetFlagPosition(flag);
+ return (flags[byte_num] & (1 << bit_num)) != 0;
+}
+
+void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag) {
+ auto [byte_num, bit_num] = GetFlagPosition(flag);
+ flags[byte_num] |= (1 << bit_num);
+}
+
+void WriteInt32LittleEndian(int32_t value, std::span<uint8_t, 4> output) {
+ WriteLittleEndian(value, output.data());
+}
+
+int32_t ReadInt32LittleEndian(std::span<const uint8_t, 4> input) {
+ return ReadLittleEndian<int32_t>(input.data());
+}
+
+int32_t ReadInt32LittleEndian(std::span<const uint8_t> data, int32_t offset) {
+ ICEBERG_DCHECK(offset >= 0, "Offset must be non-negative");
Review Comment:
ICEBERG_DCHECK is a debug check and should only be used in a rare case. If this is a generic function and cannot guarantee the input, we should return `Result<int32_t>` and replace it by `ICEBERG_PRECHECK` here.
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
Review Comment:
```suggestion
static constexpr std::array<uint8_t, 4> kMagicV1 = {0x50, 0x46, 0x41, 0x31};
```
Let's make V1 explicit.
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
Review Comment:
It is not the right place to put compression logic here as this can be shared by TableMetadata compression as well. We can consider a single place to unify the codec implementation (as well as the codec availability).
##########
src/iceberg/puffin/puffin_format.cc:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/puffin/puffin_format.h"
+
+#include <utility>
+
+#include "iceberg/util/endian.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg::puffin {
+
+namespace {
+
+constexpr std::pair<int, int> GetFlagPosition(PuffinFlag flag) {
Review Comment:
Add a comment to improve the readability?
##########
src/iceberg/test/puffin_json_test.cc:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <string>
+
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/puffin/puffin_json_internal.h"
+
+namespace iceberg::puffin {
+
+TEST(PuffinJsonTest, BlobMetadataRoundTrip) {
+ BlobMetadata blob;
+ blob.type = "apache-datasketches-theta-v1";
+ blob.input_fields = {1, 2};
+ blob.snapshot_id = 12345;
+ blob.sequence_number = 67;
+ blob.offset = 100;
+ blob.length = 200;
+ blob.compression_codec = "zstd";
+ blob.properties = {{"key", "value"}};
+
+ nlohmann::json expected_json = R"({
+ "type": "apache-datasketches-theta-v1",
+ "fields": [1, 2],
+ "snapshot-id": 12345,
+ "sequence-number": 67,
+ "offset": 100,
+ "length": 200,
+ "compression-codec": "zstd",
+ "properties": {"key": "value"}
+ })"_json;
+
+ EXPECT_EQ(ToJson(blob), expected_json);
+
+ auto result = BlobMetadataFromJson(expected_json);
+ ASSERT_TRUE(result.has_value());
+ EXPECT_EQ(result.value(), blob);
Review Comment:
nit: merge these two checks into `ASSERT_THAT`
##########
src/iceberg/test/puffin_json_test.cc:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <string>
+
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/puffin/puffin_json_internal.h"
+
+namespace iceberg::puffin {
+
+TEST(PuffinJsonTest, BlobMetadataRoundTrip) {
Review Comment:
We can improve these test cases by refactoring them into parameterized test with more valid and invalid cases. A lot of other test files have done similar jobs.
##########
src/iceberg/test/puffin_json_test.cc:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <string>
+
+#include <gtest/gtest.h>
+#include <nlohmann/json.hpp>
+
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/puffin/puffin_json_internal.h"
+
+namespace iceberg::puffin {
+
+TEST(PuffinJsonTest, BlobMetadataRoundTrip) {
+ BlobMetadata blob;
Review Comment:
nit: use aggregate initialization.
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r3069671851
##########
src/iceberg/puffin/json_serde_internal.h:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/json_serde_internal.h
+/// JSON serialization/deserialization for Puffin file metadata.
+
+#include <string>
+#include <string_view>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Serialize a BlobMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const BlobMetadata& blob_metadata);
+
+/// \brief Deserialize a BlobMetadata from JSON.
+ICEBERG_EXPORT Result<BlobMetadata> BlobMetadataFromJson(const nlohmann::json& json);
+
+/// \brief Serialize a FileMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const FileMetadata& file_metadata);
+
+/// \brief Deserialize a FileMetadata from JSON.
+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,
Review Comment:
ToJsonString/FileMetadataFromJsonString are convenience wrappers for the reader/writer — they handle the json::parse/json::dump + error wrapping that would otherwise be duplicated at each call site. Happy to remove if you prefer callers to use ToJson().dump() directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2990751540
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
Review Comment:
There is very code for this in the roaring bitmap code it should probably standardized in a common place?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r3004091026
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2990800676
##########
src/iceberg/puffin/puffin_json_internal.h:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_json_internal.h
+/// JSON serialization/deserialization for Puffin file metadata.
+
+#include <string>
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Serialize a BlobMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const BlobMetadata& blob_metadata);
+
+/// \brief Deserialize a BlobMetadata from JSON.
+ICEBERG_EXPORT Result<BlobMetadata> BlobMetadataFromJson(const nlohmann::json& json);
+
+/// \brief Serialize a FileMetadata to JSON.
+ICEBERG_EXPORT nlohmann::json ToJson(const FileMetadata& file_metadata);
+
+/// \brief Deserialize a FileMetadata from JSON.
+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,
+ bool pretty = false);
+
+/// \brief Deserialize a FileMetadata from a JSON string.
+ICEBERG_EXPORT Result<FileMetadata> FileMetadataFromJsonString(
+ const std::string& json_string);
Review Comment:
nit: std::string_view?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r2990816611
##########
src/iceberg/test/puffin_test.cc:
##########
Review Comment:
seems like tests should be seperated per .cc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(puffin): add format constants, utilities, and JSON serialization [iceberg-cpp]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #603:
URL: https://github.com/apache/iceberg-cpp/pull/603#discussion_r3003097364
##########
src/iceberg/puffin/puffin_format.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/puffin_format.h
+/// Puffin file format constants and utilities.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Puffin file format constants.
+struct ICEBERG_EXPORT PuffinFormat {
+ /// Magic bytes: "PFA1" (Puffin Fratercula arctica, version 1)
+ static constexpr std::array<uint8_t, 4> kMagic = {0x50, 0x46, 0x41, 0x31};
+
+ static constexpr int32_t kMagicLength = 4;
+ static constexpr int32_t kFooterStartMagicOffset = 0;
+ static constexpr int32_t kFooterStartMagicLength = kMagicLength;
+ static constexpr int32_t kFooterStructPayloadSizeOffset = 0;
+ static constexpr int32_t kFooterStructFlagsOffset = kFooterStructPayloadSizeOffset + 4;
+ static constexpr int32_t kFooterStructFlagsLength = 4;
+ static constexpr int32_t kFooterStructMagicOffset =
+ kFooterStructFlagsOffset + kFooterStructFlagsLength;
+
+ /// Total length of the footer struct: payload_size(4) + flags(4) + magic(4)
+ static constexpr int32_t kFooterStructLength = kFooterStructMagicOffset + kMagicLength;
+
+ /// Default compression codec for footer payload.
+ static constexpr PuffinCompressionCodec kFooterCompressionCodec =
+ PuffinCompressionCodec::kLz4;
+};
+
+/// \brief Footer flags for Puffin files.
+enum class PuffinFlag : uint8_t {
+ /// Whether the footer payload is compressed.
+ kFooterPayloadCompressed = 0,
+};
+
+/// \brief Check if a flag is set in the flags bytes.
+ICEBERG_EXPORT bool IsFlagSet(std::span<const uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Set a flag in the flags bytes.
+ICEBERG_EXPORT void SetFlag(std::span<uint8_t, 4> flags, PuffinFlag flag);
+
+/// \brief Write a 32-bit integer in little-endian format.
+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);
+
+/// \brief Read a 32-bit integer from a buffer at the given offset in little-endian
+/// format.
+ICEBERG_EXPORT int32_t ReadInt32LittleEndian(std::span<const uint8_t> data,
+ int32_t offset);
+
+/// \brief Compress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Compress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
+
+/// \brief Decompress data using the specified codec.
+ICEBERG_EXPORT Result<std::vector<uint8_t>> Decompress(PuffinCompressionCodec codec,
+ std::span<const uint8_t> input);
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org