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/11 08:24:00 UTC
[PR] feat(puffin): add basic data structures and constants [iceberg-cpp]
zhaoxuan1994 opened a new pull request, #588:
URL: https://github.com/apache/iceberg-cpp/pull/588
Add the foundational types for Puffin file format support:
- Blob, BlobMetadata, FileMetadata structs
- PuffinCompressionCodec enum with codec name conversion
- StandardBlobTypes and StandardPuffinProperties constants
- ToString functions for all types
- 24 unit tests covering all public APIs
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929607023
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
+ int64_t length;
+ /// Compression codec name, or std::nullopt if uncompressed.
+ std::optional<std::string> compression_codec;
Review Comment:
BlobMetadata is a direct mapping of the Puffin footer JSON, where compression-codec is a JSON string (or absent). The Java implementation also uses @Nullable String here. The conversion from string to PuffinCompressionCodec enum happens at a higher level via PuffinCompressionCodecFromName(), which returns Result<> to properly handle unknown codecs. This layering keeps the metadata faithful to the on-disk format and provides forward compatibility — if a future spec version adds a new codec, we can still deserialize the footer successfully and fail gracefully at decompression time rather than at parsing time.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588
--
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 basic data structures and constants [iceberg-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2916713448
##########
src/iceberg/CMakeLists.txt:
##########
@@ -62,6 +62,10 @@ set(ICEBERG_SOURCES
partition_field.cc
partition_spec.cc
partition_summary.cc
+ puffin/blob.cc
+ puffin/blob_metadata.cc
+ puffin/file_metadata.cc
+ puffin/puffin_compression_codec.cc
Review Comment:
The project also has a Meson build (`src/iceberg/meson.build`) that explicitly enumerates library sources/installed headers, but it is not updated to include the new `puffin/*.cc` sources or `iceberg/puffin/*.h` headers. As-is, Meson builds will miss Puffin support even though CMake builds include it; please add the corresponding entries (and `subdir('puffin')` if needed) to keep build systems in sync.
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return std::nullopt;
+ case PuffinCompressionCodec::kLz4:
+ return kLz4CodecName;
+ case PuffinCompressionCodec::kZstd:
+ return kZstdCodecName;
+ }
+ std::unreachable();
+}
Review Comment:
`std::unreachable()` is used in `CodecName`, but this translation unit doesn't include `<utility>`, which is where `std::unreachable` is declared (as done in other files like `expression/expression.cc`). Add `#include <utility>` here to avoid a compile error on standard library implementations that don't pull it in transitively.
##########
src/iceberg/test/CMakeLists.txt:
##########
@@ -124,6 +124,8 @@ add_iceberg_test(util_test
add_iceberg_test(roaring_test SOURCES roaring_test.cc)
+add_iceberg_test(puffin_test SOURCES puffin_test.cc)
+
Review Comment:
Meson test definitions (`src/iceberg/test/meson.build`) list test executables explicitly and currently don't include `puffin_test.cc`. With this change, CMake builds will run Puffin tests but Meson builds will not; please add `puffin_test` to the Meson test list to keep CI coverage consistent across build systems.
##########
src/iceberg/puffin/blob.h:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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/blob.h
+/// Blob data structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <span>
Review Comment:
`#include <span>` is not used in this header. Removing it will reduce compile-time dependencies (and avoids suggesting that `Blob` exposes span-based APIs when it currently doesn't).
```suggestion
```
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929537446
##########
src/iceberg/puffin/blob.cc:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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/blob.h"
+
+#include <format>
+
+#include "iceberg/util/formatter_internal.h"
+
+namespace iceberg::puffin {
+
+std::string ToString(const Blob& blob) {
+ std::string repr = "Blob[";
+ std::format_to(std::back_inserter(repr), "type='{}',inputFields={},", blob.type,
+ blob.input_fields);
+ std::format_to(std::back_inserter(repr), "snapshotId={},sequenceNumber={},",
+ blob.snapshot_id, blob.sequence_number);
+ std::format_to(std::back_inserter(repr), "dataSize={}", blob.data.size());
Review Comment:
That's an interesting idea! Currently Blob doesn't carry a checksum field (the Puffin spec doesn't include one either), so there's nothing available to display here. For equality comparison, Blob already has operator==. If we want to add checksum support down the road, it would probably be a separate discussion about the struct design itself — happy to explore that if there's a use case!
--
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 basic data structures and constants [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2973774329
##########
src/iceberg/puffin/file_metadata.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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/file_metadata.h
+/// Data structures for Puffin files: Blob, BlobMetadata, and FileMetadata.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/puffin_compression_codec.h"
+
+namespace iceberg::puffin {
+
+/// \brief A blob in a Puffin file.
+struct ICEBERG_EXPORT Blob {
+ /// \brief Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// \brief List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// \brief ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// \brief Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// \brief The uncompressed blob data.
+ std::vector<uint8_t> data;
+ /// \brief Requested compression codec. If not set, the writer's default will be used.
+ std::optional<PuffinCompressionCodec> requested_compression;
Review Comment:
It seems that we can also include both `types.h` and `puffin_compression_codec.h` to this file as they are unlikely to be used individually.
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+#include <utility>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::string_view CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return "";
+ case PuffinCompressionCodec::kLz4:
+ return kLz4CodecName;
+ case PuffinCompressionCodec::kZstd:
+ return kZstdCodecName;
+ }
+ std::unreachable();
+}
+
+Result<PuffinCompressionCodec> PuffinCompressionCodecFromName(
+ std::string_view codec_name) {
+ if (codec_name.empty()) {
+ return PuffinCompressionCodec::kNone;
+ }
+ if (codec_name == kLz4CodecName) {
+ return PuffinCompressionCodec::kLz4;
+ }
+ if (codec_name == kZstdCodecName) {
+ return PuffinCompressionCodec::kZstd;
+ }
+ return InvalidArgument("Unknown codec name: {}", codec_name);
+}
+
+std::string ToString(PuffinCompressionCodec codec) {
+ auto name = CodecName(codec);
+ if (!name.empty()) {
+ return std::string(name);
+ }
+ return "none";
Review Comment:
This breaks the roundtrip with PuffinCompressionCodecFromName. Is this intentional? Should we return empty string as is?
##########
src/iceberg/puffin/puffin_compression_codec.h:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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_compression_codec.h
+/// Compression codec definitions for Puffin files.
+
+#include <string>
+#include <string_view>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Compression codecs supported by Puffin files.
+enum class PuffinCompressionCodec {
+ /// No compression
+ kNone,
+ /// LZ4 single compression frame with content size present
+ kLz4,
+ /// Zstandard single compression frame with content size present
+ kZstd,
+};
+
+/// \brief Get the codec name for a compression codec.
+///
+/// Returns an empty string for kNone.
+ICEBERG_EXPORT std::string_view CodecName(PuffinCompressionCodec codec);
+
+/// \brief Get the compression codec from a codec name.
+///
+/// An empty string is interpreted as kNone.
Review Comment:
```suggestion
```
##########
src/iceberg/puffin/puffin_compression_codec.h:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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_compression_codec.h
+/// Compression codec definitions for Puffin files.
+
+#include <string>
+#include <string_view>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Compression codecs supported by Puffin files.
+enum class PuffinCompressionCodec {
+ /// No compression
+ kNone,
+ /// LZ4 single compression frame with content size present
+ kLz4,
+ /// Zstandard single compression frame with content size present
+ kZstd,
+};
+
+/// \brief Get the codec name for a compression codec.
+///
+/// Returns an empty string for kNone.
Review Comment:
```suggestion
```
##########
src/iceberg/puffin/file_metadata.cc:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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/file_metadata.h"
+
+#include <format>
+
+#include "iceberg/util/formatter_internal.h"
+
+namespace iceberg::puffin {
+
+std::string ToString(const Blob& blob) {
+ std::string repr = "Blob[";
+ std::format_to(std::back_inserter(repr), "type='{}',inputFields={},", blob.type,
+ blob.input_fields);
+ std::format_to(std::back_inserter(repr), "snapshotId={},sequenceNumber={},",
+ blob.snapshot_id, blob.sequence_number);
+ std::format_to(std::back_inserter(repr), "dataSize={}", blob.data.size());
+ if (blob.requested_compression.has_value()) {
+ std::format_to(std::back_inserter(repr), ",requestedCompression={}",
+ iceberg::puffin::ToString(*blob.requested_compression));
Review Comment:
```suggestion
ToString(*blob.requested_compression));
```
--
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 basic data structures and constants [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2960880991
##########
src/iceberg/puffin/blob.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob.h
+/// Blob data structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/puffin_compression_codec.h"
+
+namespace iceberg::puffin {
+
+/// \brief A blob to be written to a Puffin file.
+///
+/// This represents the uncompressed blob data along with its metadata.
+/// The actual compression is handled during writing.
Review Comment:
```suggestion
/// \brief A blob in a Puffin file.
```
Let's be concise and accurate. We don't need to mention either `write` or `compress` here since this is just a blob entry.
##########
src/iceberg/puffin/file_metadata.h:
##########
@@ -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.
+ */
+
+#pragma once
+
+/// \file iceberg/puffin/file_metadata.h
+/// File metadata structure for Puffin files.
+
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/blob_metadata.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a Puffin file.
Review Comment:
I'd recommend putting definitions of `Blob`, `BlobMetadata` and other puffin related classes to this file. We don't need to use a separate file for each class.
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file. If compression_codec is set, this is
+ /// the compressed size; otherwise it is the raw size.
Review Comment:
```suggestion
/// \brief Length of the blob data in the file.
```
IMO this is obvious.
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
+ int64_t length;
+ /// Compression codec name, or std::nullopt if uncompressed.
+ std::optional<std::string> compression_codec;
Review Comment:
Should we remove std::optional here since an empty string is sufficient to represent the unset state.
##########
src/iceberg/test/puffin_test.cc:
##########
@@ -0,0 +1,319 @@
+/*
+ * 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 <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "iceberg/puffin/blob.h"
+#include "iceberg/puffin/blob_metadata.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/puffin/puffin_compression_codec.h"
+#include "iceberg/puffin/types.h"
+#include "iceberg/test/matchers.h"
+
+namespace iceberg::puffin {
+
+// ============================================================================
+// PuffinCompressionCodec Tests
+// ============================================================================
+
+TEST(PuffinCompressionCodecTest, CodecName) {
+ EXPECT_EQ(CodecName(PuffinCompressionCodec::kNone), std::nullopt);
+ EXPECT_EQ(CodecName(PuffinCompressionCodec::kLz4), "lz4");
+ EXPECT_EQ(CodecName(PuffinCompressionCodec::kZstd), "zstd");
+}
+
+TEST(PuffinCompressionCodecTest, FromName) {
+ auto result_none = PuffinCompressionCodecFromName(std::nullopt);
+ ASSERT_THAT(result_none, IsOk());
+ EXPECT_EQ(result_none.value(), PuffinCompressionCodec::kNone);
+
+ auto result_lz4 = PuffinCompressionCodecFromName("lz4");
+ ASSERT_THAT(result_lz4, IsOk());
+ EXPECT_EQ(result_lz4.value(), PuffinCompressionCodec::kLz4);
+
+ auto result_zstd = PuffinCompressionCodecFromName("zstd");
+ ASSERT_THAT(result_zstd, IsOk());
+ EXPECT_EQ(result_zstd.value(), PuffinCompressionCodec::kZstd);
+}
+
+TEST(PuffinCompressionCodecTest, FromNameUnknown) {
+ EXPECT_THAT(PuffinCompressionCodecFromName("unknown"),
+ IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(PuffinCompressionCodecFromName("LZ4"),
+ IsError(ErrorKind::kInvalidArgument));
+ EXPECT_THAT(PuffinCompressionCodecFromName(""), IsError(ErrorKind::kInvalidArgument));
+}
+
+TEST(PuffinCompressionCodecTest, ToString) {
+ EXPECT_EQ(ToString(PuffinCompressionCodec::kNone), "none");
+ EXPECT_EQ(ToString(PuffinCompressionCodec::kLz4), "lz4");
+ EXPECT_EQ(ToString(PuffinCompressionCodec::kZstd), "zstd");
+}
+
+TEST(PuffinCompressionCodecTest, RoundTrip) {
+ for (auto codec : {PuffinCompressionCodec::kLz4, PuffinCompressionCodec::kZstd}) {
+ auto name = CodecName(codec);
+ ASSERT_TRUE(name.has_value());
+ auto result = PuffinCompressionCodecFromName(*name);
+ ASSERT_THAT(result, IsOk());
+ EXPECT_EQ(result.value(), codec);
+ }
+ // kNone round-trips through nullopt
+ EXPECT_EQ(CodecName(PuffinCompressionCodec::kNone), std::nullopt);
+ auto result = PuffinCompressionCodecFromName(std::nullopt);
+ ASSERT_THAT(result, IsOk());
+ EXPECT_EQ(result.value(), PuffinCompressionCodec::kNone);
+}
+
+// ============================================================================
+// Blob Tests
+// ============================================================================
+
+TEST(BlobTest, Equality) {
Review Comment:
TBH, I think test cases like this are unnecessary.
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+#include <utility>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return std::nullopt;
Review Comment:
Why not returning an empty string? Then you don't need to wrap the return type with optional.
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+#include <utility>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return std::nullopt;
+ case PuffinCompressionCodec::kLz4:
+ return kLz4CodecName;
+ case PuffinCompressionCodec::kZstd:
+ return kZstdCodecName;
+ }
+ std::unreachable();
+}
+
+Result<PuffinCompressionCodec> PuffinCompressionCodecFromName(
+ std::optional<std::string_view> codec_name) {
Review Comment:
Same question as above.
##########
src/iceberg/puffin/blob.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob.h
+/// Blob data structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/puffin/puffin_compression_codec.h"
+
+namespace iceberg::puffin {
+
+/// \brief A blob to be written to a Puffin file.
+///
+/// This represents the uncompressed blob data along with its metadata.
+/// The actual compression is handled during writing.
+struct ICEBERG_EXPORT Blob {
+ /// Type of the blob. See StandardBlobTypes for known types.
Review Comment:
```suggestion
/// \brief Type of the blob. See StandardBlobTypes for known types.
```
Let's consistently use the doxygen style comment
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929400719
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
Review Comment:
Yes, exactly. If **compression_codec** has a value, the blob is compressed; if it's `std::nullopt`, it's uncompressed. I'll improve the comment to make this clearer.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "evindj (via GitHub)" <gi...@apache.org>.
evindj commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2928851856
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+#include <utility>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return std::nullopt;
+ case PuffinCompressionCodec::kLz4:
+ return kLz4CodecName;
+ case PuffinCompressionCodec::kZstd:
+ return kZstdCodecName;
+ }
+ std::unreachable();
Review Comment:
consider returning Result<> so we can avoid throwing an exception 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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929523444
##########
src/iceberg/puffin/types.h:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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/types.h
+/// Standard blob types and properties for Puffin files.
+
+#include <string_view>
+
+namespace iceberg::puffin {
+
+/// \brief Standard blob types defined by the Iceberg specification.
+struct StandardBlobTypes {
+ /// A serialized form of a "compact" Theta sketch produced by the
+ /// Apache DataSketches library.
+ static constexpr std::string_view kApacheDatasketchesThetaV1 =
+ "apache-datasketches-theta-v1";
+
+ /// A serialized deletion vector according to the Iceberg spec.
+ static constexpr std::string_view kDeletionVectorV1 = "deletion-vector-v1";
+};
Review Comment:
Per the Puffin spec, type is a "JSON string" field. The spec lists known blob types under "Blob types" but doesn't restrict the field to only those values — new types can be added as the spec evolves. Both the Java and Rust implementations use String for this field rather than an enum. StandardBlobTypes provides well-known constants to avoid typos while keeping the field open for future extensibility.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929400719
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
Review Comment:
Yes, exactly. If **compression_codec** has a value, the blob is compressed; if it's `std::nullopt`, it's uncompressed.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929550998
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
Review Comment:
Same reasoning as StandardBlobTypes — the spec requires this to be an open string.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "evindj (via GitHub)" <gi...@apache.org>.
evindj commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2928796644
##########
src/iceberg/puffin/blob.cc:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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/blob.h"
+
+#include <format>
+
+#include "iceberg/util/formatter_internal.h"
+
+namespace iceberg::puffin {
+
+std::string ToString(const Blob& blob) {
+ std::string repr = "Blob[";
+ std::format_to(std::back_inserter(repr), "type='{}',inputFields={},", blob.type,
+ blob.input_fields);
+ std::format_to(std::back_inserter(repr), "snapshotId={},sequenceNumber={},",
+ blob.snapshot_id, blob.sequence_number);
+ std::format_to(std::back_inserter(repr), "dataSize={}", blob.data.size());
Review Comment:
Nit: may be add a checksum if available for fast comparison(I do understand checksum is not part of the puffin spec).
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
Review Comment:
It is weird that the type is declared as string. it makes it harder to understand what type of blobs are accepted but harder to catch typos.
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
Review Comment:
How to check if the blob is compressed, Is it if the compression_codec is set?
##########
src/iceberg/puffin/types.h:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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/types.h
+/// Standard blob types and properties for Puffin files.
+
+#include <string_view>
+
+namespace iceberg::puffin {
+
+/// \brief Standard blob types defined by the Iceberg specification.
+struct StandardBlobTypes {
+ /// A serialized form of a "compact" Theta sketch produced by the
+ /// Apache DataSketches library.
+ static constexpr std::string_view kApacheDatasketchesThetaV1 =
+ "apache-datasketches-theta-v1";
+
+ /// A serialized deletion vector according to the Iceberg spec.
+ static constexpr std::string_view kDeletionVectorV1 = "deletion-vector-v1";
+};
Review Comment:
Should we do enum + toString ?
##########
src/iceberg/puffin/blob_metadata.h:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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/blob_metadata.h
+/// Blob metadata structure for Puffin files.
+
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg::puffin {
+
+/// \brief Metadata about a blob stored in a Puffin file.
+///
+/// This represents the metadata stored in the Puffin file footer,
+/// including the blob's location within the file.
+struct ICEBERG_EXPORT BlobMetadata {
+ /// Type of the blob. See StandardBlobTypes for known types.
+ std::string type;
+ /// List of field IDs the blob was computed for.
+ std::vector<int32_t> input_fields;
+ /// ID of the Iceberg table's snapshot the blob was computed from.
+ int64_t snapshot_id;
+ /// Sequence number of the Iceberg table's snapshot the blob was computed from.
+ int64_t sequence_number;
+ /// Offset in the file where the blob data starts.
+ int64_t offset;
+ /// Length of the blob data in the file (after compression, if compressed).
+ int64_t length;
+ /// Compression codec name, or std::nullopt if uncompressed.
+ std::optional<std::string> compression_codec;
Review Comment:
ditto same pattern string or enum
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929537446
##########
src/iceberg/puffin/blob.cc:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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/blob.h"
+
+#include <format>
+
+#include "iceberg/util/formatter_internal.h"
+
+namespace iceberg::puffin {
+
+std::string ToString(const Blob& blob) {
+ std::string repr = "Blob[";
+ std::format_to(std::back_inserter(repr), "type='{}',inputFields={},", blob.type,
+ blob.input_fields);
+ std::format_to(std::back_inserter(repr), "snapshotId={},sequenceNumber={},",
+ blob.snapshot_id, blob.sequence_number);
+ std::format_to(std::back_inserter(repr), "dataSize={}", blob.data.size());
Review Comment:
Checksum is not part of the Puffin spec or the Blob struct, so there's nothing to display here. For comparison, Blob already has operator==. If we want checksum support in the future, it would be a separate design discussion for the struct itself.
--
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 basic data structures and constants [iceberg-cpp]
Posted by "zhaoxuan1994 (via GitHub)" <gi...@apache.org>.
zhaoxuan1994 commented on code in PR #588:
URL: https://github.com/apache/iceberg-cpp/pull/588#discussion_r2929645632
##########
src/iceberg/puffin/puffin_compression_codec.cc:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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_compression_codec.h"
+
+#include <format>
+#include <utility>
+
+namespace iceberg::puffin {
+
+namespace {
+constexpr std::string_view kLz4CodecName = "lz4";
+constexpr std::string_view kZstdCodecName = "zstd";
+} // namespace
+
+std::optional<std::string_view> CodecName(PuffinCompressionCodec codec) {
+ switch (codec) {
+ case PuffinCompressionCodec::kNone:
+ return std::nullopt;
+ case PuffinCompressionCodec::kLz4:
+ return kLz4CodecName;
+ case PuffinCompressionCodec::kZstd:
+ return kZstdCodecName;
+ }
+ std::unreachable();
Review Comment:
std::unreachable() here isn't actually throwing — it's a C++23 compile-time hint that this code path is impossible, since the switch already covers all enum values. This is a common pattern in the codebase (e.g., transform.cc, type.cc, expression.cc, etc.). Returning Result<> would require callers to handle an error that can't happen in practice. That said, PuffinCompressionCodecFromName does return Result<> since its string input can genuinely be invalid — so the two functions intentionally use different strategies based on their input types.
--
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