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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
