wgtmac commented on code in PR #777:
URL: https://github.com/apache/iceberg-cpp/pull/777#discussion_r3478616924
##########
src/iceberg/puffin/puffin_writer.cc:
##########
@@ -46,6 +48,14 @@ Result<std::unique_ptr<PuffinWriter>> PuffinWriter::Make(
PuffinCompressionCodec default_codec, bool compress_footer) {
ICEBERG_PRECHECK(output_file, "Output file must not be null");
ICEBERG_ASSIGN_OR_RAISE(auto stream, output_file->Create());
+ // Identify the writer in the footer unless the caller already set it. Only
+ // format the default value when it is actually needed.
+ const std::string created_by_key(StandardPuffinProperties::kCreatedBy);
+ if (!properties.contains(created_by_key)) {
Review Comment:
Please do not set created-by in the generic PuffinWriter. Java only sets it
in BaseDVFileWriter when writing DVs. Keeping it here changes footer metadata
for every Puffin writer.
##########
src/iceberg/data/deletion_vector_writer.cc:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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/data/deletion_vector_writer.h"
+
+#include <map>
+#include <optional>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "iceberg/deletes/roaring_position_bitmap.h"
+#include "iceberg/file_io.h"
+#include "iceberg/manifest/manifest_entry.h"
+#include "iceberg/partition_spec.h"
+#include "iceberg/puffin/deletion_vector.h"
+#include "iceberg/puffin/puffin_writer.h"
+#include "iceberg/util/macros.h"
+
+namespace iceberg {
+
+class DeletionVectorWriter::Impl {
+ public:
+ explicit Impl(DeletionVectorWriterOptions options) :
options_(std::move(options)) {}
+
+ Status Delete(std::string_view referenced_data_file, int64_t pos) {
+ ICEBERG_CHECK(!closed_, "Cannot delete after the writer is closed");
+ ICEBERG_PRECHECK(!referenced_data_file.empty(),
+ "Deletion vector requires a non-empty referenced data
file");
+ ICEBERG_PRECHECK(pos >= 0 && pos <= RoaringPositionBitmap::kMaxPosition,
+ "Invalid deletion vector position: {}", pos);
+ bitmaps_[std::string(referenced_data_file)].Add(pos);
+ return {};
+ }
+
+ Status Close() {
+ if (closed_) {
+ return {};
+ }
+
+ // No deletes: skip creating an orphan Puffin file that no metadata would
+ // reference, matching the Java DV writer.
+ if (bitmaps_.empty()) {
+ closed_ = true;
+ return {};
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(auto output_file,
options_.io->NewOutputFile(options_.path));
+ ICEBERG_ASSIGN_OR_RAISE(
+ auto writer,
+ puffin::PuffinWriter::Make(std::move(output_file),
options_.properties));
+
+ // One blob per referenced data file, in deterministic (sorted) order.
+ struct Entry {
+ std::string referenced_data_file;
+ int64_t offset;
+ int64_t length;
+ int64_t cardinality;
+ };
+ std::vector<Entry> entries;
+ entries.reserve(bitmaps_.size());
+ for (auto& [referenced_data_file, bitmap] : bitmaps_) {
+ // Run-length encode before serializing for space efficiency, matching
the
+ // Java DV writer.
Review Comment:
Please avoid comments that justify the code by saying it matches Java. A
short local comment is enough, or the test can cover the parity. This reads
like review explanation.
##########
src/iceberg/puffin/deletion_vector.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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/deletion_vector.h
+/// Serialization helpers for the `deletion-vector-v1` Puffin blob type.
+
+#include <array>
+#include <cstdint>
+#include <span>
+#include <string>
+#include <string_view>
+#include <vector>
+
+#include "iceberg/deletes/roaring_position_bitmap.h"
+#include "iceberg/iceberg_data_export.h"
+#include "iceberg/puffin/file_metadata.h"
+#include "iceberg/result.h"
+
+namespace iceberg::puffin {
+
+/// \brief Required blob properties for the `deletion-vector-v1` blob type.
+struct StandardDeletionVectorProperties {
+ /// Location of the data file the deletion vector applies to.
+ static constexpr std::string_view kReferencedDataFile =
"referenced-data-file";
+ /// Number of deleted rows (set positions) in the deletion vector.
+ static constexpr std::string_view kCardinality = "cardinality";
+};
+
+/// \brief Constants describing the `deletion-vector-v1` blob framing.
+///
+/// The serialized blob has the following layout (see the Puffin spec):
Review Comment:
This is too much spec text for a public header. Please keep the header brief
and point to the Puffin spec. The byte layout is better covered in the
implementation or tests.
##########
src/iceberg/data/deletion_vector_writer.h:
##########
@@ -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.
+ */
+
+#pragma once
+
+/// \file iceberg/data/deletion_vector_writer.h
+/// Writer that emits deletion vectors as `deletion-vector-v1` blobs in a
Puffin file.
+
+#include <cstdint>
+#include <memory>
+#include <string>
+#include <string_view>
+#include <unordered_map>
+
+#include "iceberg/data/writer.h"
+#include "iceberg/iceberg_data_export.h"
+#include "iceberg/result.h"
+#include "iceberg/row/partition_values.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Options for creating a DeletionVectorWriter.
+struct ICEBERG_DATA_EXPORT DeletionVectorWriterOptions {
+ /// Output Puffin file location.
+ std::string path;
+ /// FileIO used to create the Puffin file.
+ std::shared_ptr<FileIO> io;
+ /// Partition spec the referenced data files belong to (optional).
+ std::shared_ptr<PartitionSpec> spec;
+ /// Partition the referenced data files belong to.
+ PartitionValues partition;
+ /// File-level Puffin properties (e.g. "created-by").
+ std::unordered_map<std::string, std::string> properties;
+};
+
+/// \brief Writes one or more deletion vectors into a single Puffin file.
+///
+/// Each referenced data file gets its own `deletion-vector-v1` blob. After
+/// Close(), Metadata() returns one DataFile per blob, each carrying the
+/// content_offset/content_size_in_bytes and referenced_data_file required to
+/// register the deletion vector in a manifest.
+///
+/// \note All referenced data files are assumed to belong to the single
Review Comment:
Please remove this note once the API is fixed. It documents a limitation
that Java does not have and that the spec does not require.
--
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]
