wgtmac commented on code in PR #777: URL: https://github.com/apache/iceberg-cpp/pull/777#discussion_r3478592038
########## 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; Review Comment: Please store the partition spec and partition per referenced data file, not per writer. Java DVFileWriter passes them on each delete call, and the spec allows one Puffin file to hold DVs for files from different partitions. With this API, one writer can emit the wrong delete-file metadata. ########## 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_) { Review Comment: This should merge existing deletes before writing a DV for a path. The spec requires at most one DV per data file, and Java BaseDVFileWriter loads previous DVs/position deletes and reports rewritten delete files. As-is, adding another DV can leave stale delete files in table state. ########## 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 keep this default in the DV writer path instead of the generic PuffinWriter. Java does not add created-by in Puffin.write(...).build(); BaseDVFileWriter sets it when creating DV files. This changes footer metadata for all Puffin writers. -- 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]
