From e03d8d09a00ef56cfa51874007543e440765f7f4 Mon Sep 17 00:00:00 2001 From: "xuan.zhao" Date: Thu, 11 Jun 2026 19:06:05 +0800 Subject: [PATCH 1/3] feat(puffin): support deletion-vector-v1 blob read/write --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/data/delete_loader.cc | 115 +++++- src/iceberg/data/delete_loader.h | 6 +- src/iceberg/data/deletion_vector_writer.cc | 243 +++++++++++ src/iceberg/data/deletion_vector_writer.h | 97 +++++ src/iceberg/data/meson.build | 1 + src/iceberg/data/writer.h | 15 + src/iceberg/deletes/position_delete_index.cc | 148 +++++++ src/iceberg/deletes/position_delete_index.h | 36 ++ src/iceberg/meson.build | 1 + src/iceberg/test/CMakeLists.txt | 3 + src/iceberg/test/delete_filter_test.cc | 44 +- src/iceberg/test/delete_loader_test.cc | 143 ++++++- .../test/deletion_vector_writer_test.cc | 376 ++++++++++++++++++ .../test/file_scan_task_reader_test.cc | 44 ++ src/iceberg/test/meson.build | 4 + .../test/position_delete_index_test.cc | 149 +++++++ 17 files changed, 1403 insertions(+), 23 deletions(-) create mode 100644 src/iceberg/data/deletion_vector_writer.cc create mode 100644 src/iceberg/data/deletion_vector_writer.h create mode 100644 src/iceberg/test/deletion_vector_writer_test.cc diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 049a61256..d8787892e 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -196,6 +196,7 @@ set(ICEBERG_DATA_SOURCES data/data_writer.cc data/delete_filter.cc data/delete_loader.cc + data/deletion_vector_writer.cc data/equality_delete_writer.cc data/file_scan_task_reader.cc data/position_delete_writer.cc diff --git a/src/iceberg/data/delete_loader.cc b/src/iceberg/data/delete_loader.cc index 922173401..ad9207f5e 100644 --- a/src/iceberg/data/delete_loader.cc +++ b/src/iceberg/data/delete_loader.cc @@ -19,9 +19,12 @@ #include "iceberg/data/delete_loader.h" +#include #include +#include #include #include +#include #include #include @@ -30,12 +33,14 @@ #include "iceberg/arrow_c_data_guard_internal.h" #include "iceberg/deletes/position_delete_index.h" #include "iceberg/deletes/position_delete_range_consumer.h" +#include "iceberg/file_io.h" #include "iceberg/file_reader.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/metadata_columns.h" #include "iceberg/result.h" #include "iceberg/row/arrow_array_wrapper.h" #include "iceberg/schema.h" +#include "iceberg/util/content_file_util.h" #include "iceberg/util/macros.h" #include "iceberg/util/struct_like_set.h" @@ -89,10 +94,11 @@ DeleteLoader::DeleteLoader(std::shared_ptr io) : io_(std::move(io)) {} DeleteLoader::~DeleteLoader() = default; -Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index, +Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, + PositionDeleteIndex& index, std::string_view data_file_path) const { // TODO(gangwu): push down path filter to open the file. - ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(file, PosDeleteSchema(), io_)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(*file, PosDeleteSchema(), io_)); ICEBERG_ASSIGN_OR_RAISE(auto arrow_schema, reader->Schema()); internal::ArrowSchemaGuard schema_guard(&arrow_schema); @@ -110,8 +116,8 @@ Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteInde // `ForEachPositionDelete`. Trusts the hint -- spec-compliant writers // only set it when all rows share one data file. const bool use_referenced_data_file_fast_path = - file.referenced_data_file.has_value() && - file.referenced_data_file.value() == data_file_path; + file->referenced_data_file.has_value() && + file->referenced_data_file.value() == data_file_path; // Filter-path staging buffer; reused across batches via `clear()`. std::vector positions; @@ -119,6 +125,10 @@ Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteInde // reused across batches and across both routing branches. std::vector bulk_scratch; + // Whether any position for the target data file came from this file, so the + // source delete file is recorded once. + bool contributed = false; + while (true) { ICEBERG_ASSIGN_OR_RAISE(auto batch_opt, reader->Next()); if (!batch_opt.has_value()) break; @@ -150,6 +160,9 @@ Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteInde const int64_t* pos_data = Int64ValuesBuffer(pos_view); if (use_referenced_data_file_fast_path) { + // Reaching here means length > 0 (empty batches are skipped above), so + // this file contributes positions for the target data file. + contributed = true; ForEachPositionDelete(std::span(pos_data, length), index, bulk_scratch); continue; @@ -164,37 +177,105 @@ Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteInde positions.push_back(pos_data[i]); } } + if (!positions.empty()) { + contributed = true; + } ForEachPositionDelete(positions, index, bulk_scratch); } - return reader->Close(); + ICEBERG_RETURN_UNEXPECTED(reader->Close()); + + // Record the source delete file so callers (e.g. DeletionVectorWriter) can + // report file-scoped position deletes as rewritten. + if (contributed) { + index.AddDeleteFile(file); + } + return {}; } -Status DeleteLoader::LoadDV(const DataFile& file, PositionDeleteIndex& index) const { - return NotSupported("Loading deletion vectors is not yet supported"); +Status DeleteLoader::LoadDV(const std::shared_ptr& file, + PositionDeleteIndex& index, + std::string_view data_file_path) const { + // A deletion vector must reference exactly one data file; without it the + // caller cannot know which data file the positions apply to. + ICEBERG_PRECHECK(file->referenced_data_file.has_value(), + "Deletion vector requires referenced_data_file: {}", file->file_path); + + // The DV must reference the data file being read. A mismatch means the caller + // grouped delete files incorrectly; failing here avoids silently applying the + // wrong DV or hiding bad metadata behind an empty index. + ICEBERG_PRECHECK(file->referenced_data_file.value() == data_file_path, + "Deletion vector references {}, not the requested data file {}", + file->referenced_data_file.value(), data_file_path); + + // For deletion vectors, content_offset and content_size_in_bytes point directly + // at the DV blob bytes within the Puffin file and are required by the spec. + ICEBERG_PRECHECK( + file->content_offset.has_value() && file->content_size_in_bytes.has_value(), + "Deletion vector requires content_offset and content_size_in_bytes: {}", + file->file_path); + + const int64_t offset = file->content_offset.value(); + const int64_t length = file->content_size_in_bytes.value(); + ICEBERG_PRECHECK(offset >= 0 && length >= 0, + "Invalid deletion vector offset/length: offset={}, length={}", offset, + length); + ICEBERG_PRECHECK(length <= std::numeric_limits::max(), + "Cannot read deletion vector larger than 2GB: {}", length); + + ICEBERG_ASSIGN_OR_RAISE(auto input_file, io_->NewInputFile(file->file_path)); + ICEBERG_ASSIGN_OR_RAISE(auto stream, input_file->Open()); + + std::vector bytes(static_cast(length)); + ICEBERG_RETURN_UNEXPECTED(stream->ReadFully(offset, bytes)); + ICEBERG_RETURN_UNEXPECTED(stream->Close()); + + std::span blob(reinterpret_cast(bytes.data()), + bytes.size()); + // Deserialize validates the blob length and cardinality against `file` and + // retains it as the source delete file. + ICEBERG_ASSIGN_OR_RAISE(auto dv, PositionDeleteIndex::Deserialize(blob, file)); + + index.Merge(dv); + return {}; } Result DeleteLoader::LoadPositionDeletes( std::span> delete_files, std::string_view data_file_path) const { + for (const auto& file : delete_files) { + ICEBERG_PRECHECK(file != nullptr, "Delete file must not be null"); + } + PositionDeleteIndex index; + // A single deletion vector replaces all other deletes for a data file, so it + // is read through the dedicated DV path and validated against the requested + // data file. + if (ContentFileUtil::ContainsSingleDV(delete_files)) { + ICEBERG_RETURN_UNEXPECTED(LoadDV(delete_files.front(), index, data_file_path)); + return index; + } + + // Otherwise all entries must be position delete files. A DV must not be mixed + // with position deletes: once a DV applies, readers ignore matching position + // deletes, so a mixed list means the caller grouped delete files incorrectly. for (const auto& file : delete_files) { + ICEBERG_PRECHECK(!file->IsDeletionVector(), + "Deletion vector cannot be mixed with position delete files: {}", + file->file_path); + ICEBERG_PRECHECK(file->content == DataFile::Content::kPositionDeletes, + "Expected position delete file but got content type {}", + ToString(file->content)); + + // A file-scoped position delete for another data file has nothing for the + // target path; skip it as an optimization. if (file->referenced_data_file.has_value() && file->referenced_data_file.value() != data_file_path) { continue; } - if (file->IsDeletionVector()) { - ICEBERG_RETURN_UNEXPECTED(LoadDV(*file, index)); - continue; - } - - ICEBERG_PRECHECK(file->content == DataFile::Content::kPositionDeletes, - "Expected position delete file but got content type {}", - ToString(file->content)); - - ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(*file, index, data_file_path)); + ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(file, index, data_file_path)); } return index; diff --git a/src/iceberg/data/delete_loader.h b/src/iceberg/data/delete_loader.h index a422f0683..6bd07fd8e 100644 --- a/src/iceberg/data/delete_loader.h +++ b/src/iceberg/data/delete_loader.h @@ -67,11 +67,13 @@ class ICEBERG_DATA_EXPORT DeleteLoader { private: /// \brief Load a single position delete file into the index. - Status LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index, + Status LoadPositionDelete(const std::shared_ptr& file, + PositionDeleteIndex& index, std::string_view data_file_path) const; /// \brief Load a single deletion vector file into the index. - Status LoadDV(const DataFile& file, PositionDeleteIndex& index) const; + Status LoadDV(const std::shared_ptr& file, PositionDeleteIndex& index, + std::string_view data_file_path) const; std::shared_ptr io_; }; diff --git a/src/iceberg/data/deletion_vector_writer.cc b/src/iceberg/data/deletion_vector_writer.cc new file mode 100644 index 000000000..d4a9ca5ea --- /dev/null +++ b/src/iceberg/data/deletion_vector_writer.cc @@ -0,0 +1,243 @@ +/* + * 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 +#include +#include +#include +#include +#include + +#include "iceberg/file_io.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/metadata_columns.h" +#include "iceberg/partition_spec.h" +#include "iceberg/puffin/file_metadata.h" +#include "iceberg/puffin/puffin_writer.h" +#include "iceberg/util/content_file_util.h" +#include "iceberg/util/macros.h" +#include "iceberg/version.h" + +namespace iceberg { + +namespace { +constexpr std::string_view kReferencedDataFile = "referenced-data-file"; +constexpr std::string_view kCardinality = "cardinality"; +} // namespace + +class DeletionVectorWriter::Impl { + public: + explicit Impl(DeletionVectorWriterOptions options) : options_(std::move(options)) {} + + // Accumulated positions and metadata for a single referenced data file. + struct Deletes { + PositionDeleteIndex positions; + std::shared_ptr spec; + PartitionValues partition; + }; + + Deletes& DeletesFor(std::string_view referenced_data_file, + std::shared_ptr spec, PartitionValues partition) { + auto [it, inserted] = deletes_by_path_.try_emplace(std::string(referenced_data_file)); + if (inserted) { + it->second.spec = std::move(spec); + it->second.partition = std::move(partition); + } + return it->second; + } + + Status Delete(std::string_view referenced_data_file, int64_t pos, + std::shared_ptr spec, PartitionValues partition) { + 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, + "Deletion vector position out of range [0, {}]: {}", + RoaringPositionBitmap::kMaxPosition, pos); + DeletesFor(referenced_data_file, std::move(spec), std::move(partition)) + .positions.Delete(pos); + return {}; + } + + Status Delete(std::string_view referenced_data_file, + const PositionDeleteIndex& positions, std::shared_ptr spec, + PartitionValues partition) { + 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"); + DeletesFor(referenced_data_file, std::move(spec), std::move(partition)) + .positions.Merge(positions); + return {}; + } + + Status Close() { + if (closed_) { + return {}; + } + + // No deletes: skip creating an orphan Puffin file that no metadata + // references. + if (deletes_by_path_.empty()) { + closed_ = true; + return {}; + } + + // Merge previously written deletes and collect the file-scoped delete files + // they came from so callers can remove them from table state. + for (auto& [path, deletes] : deletes_by_path_) { + ICEBERG_ASSIGN_OR_RAISE(auto previous, options_.load_previous_deletes(path)); + if (previous == nullptr) { + continue; + } + deletes.positions.Merge(*previous); + for (const auto& delete_file : previous->delete_files()) { + ICEBERG_ASSIGN_OR_RAISE(bool file_scoped, + ContentFileUtil::IsFileScoped(*delete_file)); + if (file_scoped) { + result_.rewritten_delete_files.push_back(delete_file); + } + } + } + + auto properties = options_.properties; + if (const std::string created_by(puffin::StandardPuffinProperties::kCreatedBy); + !properties.contains(created_by)) { + properties.emplace(created_by, + std::format("iceberg-cpp/{}.{}.{}", ICEBERG_VERSION_MAJOR, + ICEBERG_VERSION_MINOR, ICEBERG_VERSION_PATCH)); + } + + 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), std::move(properties))); + + struct Entry { + std::string referenced_data_file; + std::shared_ptr spec; + PartitionValues partition; + int64_t offset; + int64_t length; + int64_t cardinality; + }; + std::vector entries; + entries.reserve(deletes_by_path_.size()); + + for (auto& [path, deletes] : deletes_by_path_) { + const int64_t cardinality = deletes.positions.Cardinality(); + ICEBERG_ASSIGN_OR_RAISE(auto data, deletes.positions.Serialize()); + + puffin::Blob blob{ + .type = std::string(puffin::StandardBlobTypes::kDeletionVectorV1), + .input_fields = {MetadataColumns::kFilePositionColumnId}, + // Snapshot ID and sequence number are inherited; the spec requires -1. + .snapshot_id = -1, + .sequence_number = -1, + .data = std::move(data), + .requested_compression = puffin::PuffinCompressionCodec::kNone, + }; + blob.properties.emplace(std::string(kReferencedDataFile), path); + blob.properties.emplace(std::string(kCardinality), std::format("{}", cardinality)); + + ICEBERG_ASSIGN_OR_RAISE(auto blob_metadata, writer->Write(blob)); + entries.push_back(Entry{ + .referenced_data_file = path, + .spec = deletes.spec, + .partition = deletes.partition, + .offset = blob_metadata.offset, + .length = blob_metadata.length, + .cardinality = cardinality, + }); + } + + ICEBERG_RETURN_UNEXPECTED(writer->Finish()); + ICEBERG_ASSIGN_OR_RAISE(const int64_t file_size, writer->FileSize()); + + for (auto& entry : entries) { + result_.referenced_data_files.push_back(entry.referenced_data_file); + result_.delete_files.push_back(std::make_shared(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = options_.path, + .file_format = FileFormatType::kPuffin, + .partition = std::move(entry.partition), + .record_count = entry.cardinality, + .file_size_in_bytes = file_size, + .referenced_data_file = std::move(entry.referenced_data_file), + .content_offset = entry.offset, + .content_size_in_bytes = entry.length, + .partition_spec_id = + entry.spec ? std::make_optional(entry.spec->spec_id()) : std::nullopt, + })); + } + + closed_ = true; + return {}; + } + + Result Metadata() { + ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer"); + return result_; + } + + private: + DeletionVectorWriterOptions options_; + // Ordered by referenced data file path for deterministic blob layout. + std::map deletes_by_path_; + DeleteWriteResult result_; + bool closed_ = false; +}; + +DeletionVectorWriter::DeletionVectorWriter(std::unique_ptr impl) + : impl_(std::move(impl)) {} + +DeletionVectorWriter::~DeletionVectorWriter() = default; + +Result> DeletionVectorWriter::Make( + DeletionVectorWriterOptions options) { + ICEBERG_PRECHECK(options.io != nullptr, "DeletionVectorWriter requires a FileIO"); + ICEBERG_PRECHECK(!options.path.empty(), "DeletionVectorWriter requires a path"); + ICEBERG_PRECHECK( + static_cast(options.load_previous_deletes), + "DeletionVectorWriter requires a load_previous_deletes callback (return " + "nullptr when a data file has no existing deletes)"); + return std::unique_ptr( + new DeletionVectorWriter(std::make_unique(std::move(options)))); +} + +Status DeletionVectorWriter::Delete(std::string_view referenced_data_file, int64_t pos, + std::shared_ptr spec, + PartitionValues partition) { + return impl_->Delete(referenced_data_file, pos, std::move(spec), std::move(partition)); +} + +Status DeletionVectorWriter::Delete(std::string_view referenced_data_file, + const PositionDeleteIndex& positions, + std::shared_ptr spec, + PartitionValues partition) { + return impl_->Delete(referenced_data_file, positions, std::move(spec), + std::move(partition)); +} + +Status DeletionVectorWriter::Close() { return impl_->Close(); } + +Result DeletionVectorWriter::Metadata() { return impl_->Metadata(); } + +} // namespace iceberg diff --git a/src/iceberg/data/deletion_vector_writer.h b/src/iceberg/data/deletion_vector_writer.h new file mode 100644 index 000000000..86856b7d8 --- /dev/null +++ b/src/iceberg/data/deletion_vector_writer.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/data/deletion_vector_writer.h +/// Writer that emits deletion vectors as `deletion-vector-v1` blobs in a Puffin file. + +#include +#include +#include +#include +#include +#include +#include + +#include "iceberg/data/writer.h" +#include "iceberg/deletes/position_delete_index.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 io; + /// File-level Puffin properties (e.g. "created-by"). + std::unordered_map properties; + /// Hook to load previously written deletes for a data file so they can be + /// merged into the new deletion vector. The returned index carries its source + /// delete files (via PositionDeleteIndex::delete_files()); file-scoped ones + /// are reported as rewritten. Return nullptr when there are none. Required: a + /// DV must replace any existing deletes for its data file. + std::function>(std::string_view)> + load_previous_deletes; +}; + +/// \brief Writes one or more deletion vectors into a single Puffin file. +/// +/// Each referenced data file gets its own `deletion-vector-v1` blob with its own +/// partition spec and partition. After Close(), Metadata() returns the produced +/// delete files plus the referenced and rewritten delete files. +class ICEBERG_DATA_EXPORT DeletionVectorWriter { + public: + ~DeletionVectorWriter(); + + DeletionVectorWriter(const DeletionVectorWriter&) = delete; + DeletionVectorWriter& operator=(const DeletionVectorWriter&) = delete; + + /// \brief Create a new DeletionVectorWriter. + static Result> Make( + DeletionVectorWriterOptions options); + + /// \brief Mark a row position as deleted for the given data file. + Status Delete(std::string_view referenced_data_file, int64_t pos, + std::shared_ptr spec, PartitionValues partition); + + /// \brief Mark all positions in the given index as deleted for a data file. + Status Delete(std::string_view referenced_data_file, + const PositionDeleteIndex& positions, std::shared_ptr spec, + PartitionValues partition); + + /// \brief Write all accumulated deletion vectors to the Puffin file and close. + Status Close(); + + /// \brief The result of writing; valid only after Close(). + Result Metadata(); + + private: + class Impl; + std::unique_ptr impl_; + + explicit DeletionVectorWriter(std::unique_ptr impl); +}; + +} // namespace iceberg diff --git a/src/iceberg/data/meson.build b/src/iceberg/data/meson.build index bbb26db27..eaae8a2b4 100644 --- a/src/iceberg/data/meson.build +++ b/src/iceberg/data/meson.build @@ -20,6 +20,7 @@ install_headers( 'data_writer.h', 'delete_filter.h', 'delete_loader.h', + 'deletion_vector_writer.h', 'equality_delete_writer.h', 'file_scan_task_reader.h', 'position_delete_writer.h', diff --git a/src/iceberg/data/writer.h b/src/iceberg/data/writer.h index a6f5f063d..f47026c7e 100644 --- a/src/iceberg/data/writer.h +++ b/src/iceberg/data/writer.h @@ -24,6 +24,7 @@ #include #include +#include #include #include "iceberg/arrow_c_data.h" @@ -33,6 +34,20 @@ namespace iceberg { +/// \brief The result of writing delete files. +/// +/// Holds the delete files produced, the data files they reference, and any +/// previously written delete files that were merged and should be removed from +/// table state. +struct ICEBERG_DATA_EXPORT DeleteWriteResult { + /// Delete files produced. + std::vector> delete_files; + /// Data files referenced by the produced delete files. + std::vector referenced_data_files; + /// Previously written delete files that were merged and should be removed. + std::vector> rewritten_delete_files; +}; + /// \brief Base interface for data file writers. class ICEBERG_DATA_EXPORT FileWriter { public: diff --git a/src/iceberg/deletes/position_delete_index.cc b/src/iceberg/deletes/position_delete_index.cc index f09d0b480..92f76f109 100644 --- a/src/iceberg/deletes/position_delete_index.cc +++ b/src/iceberg/deletes/position_delete_index.cc @@ -19,8 +19,55 @@ #include "iceberg/deletes/position_delete_index.h" +#include + +#include +#include +#include +#include +#include +#include + +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/util/endian.h" +#include "iceberg/util/macros.h" + namespace iceberg { +namespace { + +// `deletion-vector-v1` blob framing. See: +// https://iceberg.apache.org/puffin-spec/#deletion-vector-v1-blob-type +constexpr std::array kMagic = {0xD1, 0xD3, 0x39, 0x64}; +constexpr int32_t kLengthPrefixBytes = 4; +constexpr int32_t kMagicBytes = 4; +constexpr int32_t kCrcBytes = 4; + +uint32_t ComputeCrc32(std::span bytes) { + uLong crc = crc32(0L, Z_NULL, 0); + crc = crc32(crc, reinterpret_cast(bytes.data()), + static_cast(bytes.size())); + return static_cast(crc); +} + +template +void WriteBigEndian(T value, uint8_t* buf) { + T be = ToBigEndian(value); + std::memcpy(buf, &be, sizeof(be)); +} + +template +T ReadBigEndian(const uint8_t* buf) { + T value; + std::memcpy(&value, buf, sizeof(value)); + return FromBigEndian(value); +} + +} // namespace + +PositionDeleteIndex::PositionDeleteIndex(RoaringPositionBitmap bitmap) + : bitmap_(std::move(bitmap)) {} + void PositionDeleteIndex::Delete(int64_t pos) { bitmap_.Add(pos); } void PositionDeleteIndex::Delete(int64_t pos_start, int64_t pos_end) { @@ -37,6 +84,107 @@ int64_t PositionDeleteIndex::Cardinality() const { void PositionDeleteIndex::Merge(const PositionDeleteIndex& other) { bitmap_.Or(other.bitmap_); + delete_files_.insert(delete_files_.end(), other.delete_files_.begin(), + other.delete_files_.end()); +} + +void PositionDeleteIndex::AddDeleteFile(std::shared_ptr delete_file) { + delete_files_.push_back(std::move(delete_file)); +} + +Result> PositionDeleteIndex::Serialize() { + bitmap_.Optimize(); // run-length encode before serializing + ICEBERG_ASSIGN_OR_RAISE(auto vector, bitmap_.Serialize()); + + // The length prefix and CRC both cover the magic sequence plus the vector. + const size_t magic_and_vector_size = static_cast(kMagicBytes) + vector.size(); + ICEBERG_PRECHECK( + magic_and_vector_size <= static_cast(std::numeric_limits::max()), + "Deletion vector is too large to serialize: {} bytes", magic_and_vector_size); + + std::vector blob(static_cast(kLengthPrefixBytes) + + magic_and_vector_size + static_cast(kCrcBytes)); + uint8_t* buf = blob.data(); + + WriteBigEndian(static_cast(magic_and_vector_size), buf); + buf += kLengthPrefixBytes; + + uint8_t* checksum_begin = buf; + std::memcpy(buf, kMagic.data(), kMagicBytes); + buf += kMagicBytes; + std::memcpy(buf, vector.data(), vector.size()); + buf += vector.size(); + + WriteBigEndian( + ComputeCrc32(std::span(checksum_begin, magic_and_vector_size)), buf); + return blob; +} + +Result PositionDeleteIndex::Deserialize( + std::span blob, std::shared_ptr delete_file) { + ICEBERG_PRECHECK(delete_file != nullptr, + "Deletion vector requires a source delete file"); + // DV metadata requires content_size_in_bytes; the blob bytes must match it. + ICEBERG_PRECHECK(delete_file->content_size_in_bytes.has_value(), + "Deletion vector requires content_size_in_bytes: {}", + delete_file->file_path); + ICEBERG_PRECHECK( + std::cmp_equal(blob.size(), delete_file->content_size_in_bytes.value()), + "Deletion vector blob size {} does not match content_size_in_bytes {}: {}", + blob.size(), delete_file->content_size_in_bytes.value(), delete_file->file_path); + + constexpr size_t kMinSize = static_cast(kLengthPrefixBytes) + + static_cast(kMagicBytes) + + static_cast(kCrcBytes); + ICEBERG_PRECHECK(blob.size() >= kMinSize, + "Deletion vector blob too small: {} bytes, need at least {}", + blob.size(), kMinSize); + + const uint8_t* buf = blob.data(); + + const auto length = ReadBigEndian(buf); + buf += kLengthPrefixBytes; + + ICEBERG_PRECHECK(length >= kMagicBytes, "Invalid deletion vector length prefix: {}", + length); + + const size_t expected_total = static_cast(kLengthPrefixBytes) + + static_cast(length) + + static_cast(kCrcBytes); + ICEBERG_PRECHECK(blob.size() == expected_total, + "Deletion vector blob size mismatch: {} bytes, expected {}", + blob.size(), expected_total); + + // Magic and vector are checksummed together by the trailing CRC. + const uint8_t* checksum_begin = buf; + for (size_t i = 0; i < kMagic.size(); ++i) { + ICEBERG_PRECHECK(buf[i] == kMagic[i], + "Invalid deletion vector magic byte at offset {}: got {:#04x}", i, + buf[i]); + } + buf += kMagicBytes; + + const auto stored_crc = + ReadBigEndian(checksum_begin + static_cast(length)); + const uint32_t actual_crc = + ComputeCrc32(std::span(checksum_begin, static_cast(length))); + ICEBERG_PRECHECK(stored_crc == actual_crc, + "Deletion vector CRC mismatch: stored {:#010x}, computed {:#010x}", + stored_crc, actual_crc); + + const auto vector_size = static_cast(length) - kMagicBytes; + std::string_view vector_bytes(reinterpret_cast(buf), vector_size); + ICEBERG_ASSIGN_OR_RAISE(auto bitmap, RoaringPositionBitmap::Deserialize(vector_bytes)); + PositionDeleteIndex index(std::move(bitmap)); + + // The bitmap cardinality must match the record count recorded in metadata. + ICEBERG_PRECHECK(std::cmp_equal(index.Cardinality(), delete_file->record_count), + "Deletion vector cardinality {} does not match record count {}: {}", + index.Cardinality(), delete_file->record_count, + delete_file->file_path); + + index.delete_files_.push_back(std::move(delete_file)); + return index; } void PositionDeleteIndex::BulkAddForKey(int32_t key, diff --git a/src/iceberg/deletes/position_delete_index.h b/src/iceberg/deletes/position_delete_index.h index 592216ed6..02c792a9d 100644 --- a/src/iceberg/deletes/position_delete_index.h +++ b/src/iceberg/deletes/position_delete_index.h @@ -29,9 +29,12 @@ #include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/iceberg_data_export.h" +#include "iceberg/result.h" namespace iceberg { +struct DataFile; + /// \brief Tracks deleted row positions using a bitmap. /// /// This class provides a domain-specific API for position deletes @@ -66,7 +69,39 @@ class ICEBERG_DATA_EXPORT PositionDeleteIndex { /// \param other The index to merge (union operation) void Merge(const PositionDeleteIndex& other); + /// \brief The delete files whose positions were merged into this index. + /// + /// Populated by Deserialize and AddDeleteFile, and preserved across Merge. + /// Callers use these to report the delete files that were rewritten when + /// replacing them with a new deletion vector. + const std::vector>& delete_files() const { + return delete_files_; + } + + /// \brief Record a delete file whose positions were merged into this index. + /// + /// Used by the position-delete load path, where positions come from a Parquet + /// delete file rather than a DV blob. The file is exposed via delete_files(). + void AddDeleteFile(std::shared_ptr delete_file); + + /// \brief Serialize the index into a `deletion-vector-v1` blob. + /// + /// The positions are run-length encoded, then framed per the Puffin spec: + /// https://iceberg.apache.org/puffin-spec/#deletion-vector-v1-blob-type + Result> Serialize(); + + /// \brief Deserialize a `deletion-vector-v1` blob into an index. + /// + /// Validates the blob framing (length prefix, magic sequence, CRC-32) and, + /// against the source delete file, that the blob length matches + /// `content_size_in_bytes` and the bitmap cardinality matches `record_count`. + /// The source delete file is retained and exposed via delete_files(). + static Result Deserialize(std::span blob, + std::shared_ptr delete_file); + private: + explicit PositionDeleteIndex(RoaringPositionBitmap bitmap); + // Bulk-add positions sharing high-32-bit `key`. Private hook for // `ForEachPositionDelete`'s bulk path; keeps `Delete` the sole public // mutation surface. @@ -77,6 +112,7 @@ class ICEBERG_DATA_EXPORT PositionDeleteIndex { std::vector& scratch); RoaringPositionBitmap bitmap_; + std::vector> delete_files_; }; } // namespace iceberg diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 7340bfa28..cc2c683e0 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -174,6 +174,7 @@ iceberg_data_sources = files( 'data/data_writer.cc', 'data/delete_filter.cc', 'data/delete_loader.cc', + 'data/deletion_vector_writer.cc', 'data/equality_delete_writer.cc', 'data/file_scan_task_reader.cc', 'data/position_delete_writer.cc', diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 7a1b45b70..a60e3ac82 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -162,6 +162,9 @@ add_iceberg_test(puffin_test puffin_json_test.cc puffin_reader_writer_test.cc) +add_iceberg_test(deletion_vector_writer_test USE_DATA SOURCES + deletion_vector_writer_test.cc) + if(ICEBERG_BUILD_BUNDLE) add_iceberg_test(avro_test USE_BUNDLE diff --git a/src/iceberg/test/delete_filter_test.cc b/src/iceberg/test/delete_filter_test.cc index 89d1b6b85..8ae8ff7e8 100644 --- a/src/iceberg/test/delete_filter_test.cc +++ b/src/iceberg/test/delete_filter_test.cc @@ -34,6 +34,7 @@ #include #include "iceberg/arrow/arrow_io_internal.h" +#include "iceberg/data/deletion_vector_writer.h" #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" #include "iceberg/file_format.h" @@ -249,6 +250,25 @@ class DeleteFilterTest : public ::testing::Test { static constexpr std::string_view kDataPath = "data.parquet"; + Result> DeletionVectorFile( + const std::string& path, const std::vector& positions, + const std::string& data_path = std::string(kDataPath)) { + DeletionVectorWriterOptions options{ + .path = path, + .io = file_io_, + .load_previous_deletes = [](std::string_view) + -> Result> { return nullptr; }, + }; + ICEBERG_ASSIGN_OR_RAISE(auto writer, DeletionVectorWriter::Make(std::move(options))); + for (int64_t pos : positions) { + ICEBERG_RETURN_UNEXPECTED( + writer->Delete(data_path, pos, partition_spec_, PartitionValues{})); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + ICEBERG_ASSIGN_OR_RAISE(auto metadata, writer->Metadata()); + return metadata.delete_files[0]; + } + std::shared_ptr file_io_; std::shared_ptr table_schema_; std::shared_ptr partition_spec_; @@ -1156,7 +1176,7 @@ TEST_F(DeleteFilterTest, DeletionVectorErrorPropagatesFromCompute) { ICEBERG_UNWRAP_OR_FAIL(auto batch, MakeBatch(*filter.value()->RequiredSchema(), R"([[1, 0]])")); auto alive = filter.value()->ComputeAliveRows(batch.schema, batch.array); - ASSERT_THAT(alive, IsError(ErrorKind::kNotSupported)); + ASSERT_THAT(alive, IsError(ErrorKind::kInvalidArgument)); } TEST_F(DeleteFilterTest, EmptyBatchPropagatesDeleteLoadErrors) { @@ -1175,7 +1195,27 @@ TEST_F(DeleteFilterTest, EmptyBatchPropagatesDeleteLoadErrors) { auto alive = filter.value()->ComputeAliveRows(batch.schema, batch.array); - ASSERT_THAT(alive, IsError(ErrorKind::kNotSupported)); + ASSERT_THAT(alive, IsError(ErrorKind::kInvalidArgument)); +} + +TEST_F(DeleteFilterTest, DeletionVectorComputeAliveRows) { + // Write a real deletion vector with DeletionVectorWriter, then load it through + // DeleteFilter (DeleteLoader::LoadDV) and verify deleted positions are filtered. + ICEBERG_UNWRAP_OR_FAIL(auto dv, DeletionVectorFile("dv-alive.puffin", {1, 3})); + std::vector> delete_files = {dv}; + auto requested_schema = Project({1}); + auto filter = DeleteFilter::Make(std::string(kDataPath), delete_files, table_schema_, + requested_schema, file_io_, + /*need_row_pos_col=*/true); + ASSERT_THAT(filter, IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto batch, + MakeBatch(*filter.value()->RequiredSchema(), + R"([[10, 0], [20, 1], [30, 2], [40, 3], [50, 4]])")); + auto alive = filter.value()->ComputeAliveRows(batch.schema, batch.array); + + ASSERT_THAT(alive, IsOk()); + ExpectAliveRows(alive.value(), {0, 2, 4}); } TEST_F(DeleteFilterTest, CounterAccumulatesAcrossBatches) { diff --git a/src/iceberg/test/delete_loader_test.cc b/src/iceberg/test/delete_loader_test.cc index b392065cf..225ef19cd 100644 --- a/src/iceberg/test/delete_loader_test.cc +++ b/src/iceberg/test/delete_loader_test.cc @@ -26,10 +26,12 @@ #include #include "iceberg/arrow/arrow_io_internal.h" +#include "iceberg/data/deletion_vector_writer.h" #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" #include "iceberg/deletes/position_delete_index.h" #include "iceberg/file_format.h" +#include "iceberg/file_io.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/parquet/parquet_register.h" #include "iceberg/partition_spec.h" @@ -110,6 +112,26 @@ class DeleteLoaderTest : public ::testing::Test { return writer->Metadata().value().data_files[0]; } + /// Write a deletion vector to a Puffin file and return the DataFile metadata. + std::shared_ptr WriteDeletionVector(const std::string& path, + const std::string& referenced_data_file, + const std::vector& positions) { + auto writer = + DeletionVectorWriter::Make( + DeletionVectorWriterOptions{ + .path = path, + .io = file_io_, + .load_previous_deletes = [](std::string_view) + -> Result> { return nullptr; }}) + .value(); + for (int64_t pos : positions) { + ICEBERG_THROW_NOT_OK( + writer->Delete(referenced_data_file, pos, partition_spec_, PartitionValues{})); + } + ICEBERG_THROW_NOT_OK(writer->Close()); + return writer->Metadata().value().delete_files[0]; + } + std::shared_ptr file_io_; std::shared_ptr schema_; std::shared_ptr partition_spec_; @@ -197,6 +219,47 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesFiltersByDataFilePath) { ASSERT_TRUE(result_none.value().IsEmpty()); } +// The loaded index keeps the source Parquet position delete file, so a DV +// writer can report file-scoped deletes as rewritten. +TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceDeleteFile) { + // Single referenced data file -> writer sets the hint -> fast path. + auto file_scoped = WritePositionDeletes("pos_src_scoped.parquet", + {{"data.parquet", 0}, {"data.parquet", 5}}); + ASSERT_TRUE(file_scoped->referenced_data_file.has_value()); + + std::vector> files = {file_scoped}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsOk()); + ASSERT_EQ(result.value().delete_files().size(), 1u); + EXPECT_EQ(result.value().delete_files()[0], file_scoped); +} + +// The filter path (mixed paths, no hint) also records the source file, but only +// when the target data file actually has matching rows. +TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceOnlyWhenMatched) { + auto mixed = WritePositionDeletes("pos_src_mixed.parquet", + {{"data_a.parquet", 0}, {"data_b.parquet", 10}}); + ASSERT_FALSE(mixed->referenced_data_file.has_value()); + + std::vector> files = {mixed}; + + auto matched = loader_->LoadPositionDeletes(files, "data_a.parquet"); + ASSERT_THAT(matched, IsOk()); + ASSERT_EQ(matched.value().delete_files().size(), 1u); + EXPECT_EQ(matched.value().delete_files()[0], mixed); + + // No matching rows -> the file is not recorded. + auto unmatched = loader_->LoadPositionDeletes(files, "data_c.parquet"); + ASSERT_THAT(unmatched, IsOk()); + EXPECT_TRUE(unmatched.value().delete_files().empty()); +} + +TEST_F(DeleteLoaderTest, LoadPositionDeletesRejectsNullFile) { + std::vector> files = {nullptr}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + TEST_F(DeleteLoaderTest, LoadPositionDeletesSkipsMismatchedReferencedDataFile) { auto delete_file = std::make_shared(DataFile{ .content = DataFile::Content::kPositionDeletes, @@ -239,15 +302,91 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesFastPathHonorsReferencedDataFile) { ASSERT_FALSE(index.IsDeleted(kRowCount)); } -TEST_F(DeleteLoaderTest, LoadPositionDeletesRejectsDV) { +TEST_F(DeleteLoaderTest, LoadDeletionVector) { + auto dv_file = + WriteDeletionVector("dv-a.puffin", "data.parquet", {0, 5, 10, 4'000'000'000LL}); + + std::vector> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsOk()); + + auto& index = result.value(); + ASSERT_EQ(index.Cardinality(), 4); + ASSERT_TRUE(index.IsDeleted(0)); + ASSERT_TRUE(index.IsDeleted(5)); + ASSERT_TRUE(index.IsDeleted(10)); + ASSERT_TRUE(index.IsDeleted(4'000'000'000LL)); + ASSERT_FALSE(index.IsDeleted(1)); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorRejectsMismatchedReferencedDataFile) { + // A DV referencing another data file must not be silently skipped: the single + // DV applies to its referenced file, so a mismatch is bad grouping/metadata. + auto dv_file = WriteDeletionVector("dv-b.puffin", "other-data.parquet", {1, 2, 3}); + + std::vector> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorRequiresContentOffsetAndSize) { auto dv_file = std::make_shared(DataFile{ .content = DataFile::Content::kPositionDeletes, .file_path = "dv.puffin", .file_format = FileFormatType::kPuffin, + .referenced_data_file = "data.parquet", }); std::vector> files = {dv_file}; auto result = loader_->LoadPositionDeletes(files, "data.parquet"); - ASSERT_THAT(result, IsError(ErrorKind::kNotSupported)); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +TEST_F(DeleteLoaderTest, LoadDeletionVectorRejectsCardinalityMismatch) { + auto dv_file = WriteDeletionVector("dv-card.puffin", "data.parquet", {0, 1, 2}); + // Corrupt the recorded cardinality so it no longer matches the bitmap. + dv_file->record_count = 99; + + std::vector> files = {dv_file}; + auto result = loader_->LoadPositionDeletes(files, "data.parquet"); + ASSERT_THAT(result, IsError(ErrorKind::kInvalidArgument)); +} + +// End to end: a previously written Parquet position delete is loaded, merged +// into a new DV, and reported in rewritten_delete_files when it is file-scoped. +TEST_F(DeleteLoaderTest, DVWriterReportsFileScopedPositionDeleteAsRewritten) { + // A file-scoped position delete (single referenced data file). + auto previous = WritePositionDeletes("prev_pos.parquet", + {{"data.parquet", 100}, {"data.parquet", 200}}); + ASSERT_TRUE(previous->referenced_data_file.has_value()); + + auto* loader = loader_.get(); + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "dv-replace.puffin", + .io = file_io_, + .load_previous_deletes = + [&](std::string_view path) -> Result> { + std::vector> files = {previous}; + ICEBERG_ASSIGN_OR_RAISE(auto index, loader->LoadPositionDeletes(files, path)); + if (index.IsEmpty()) { + return nullptr; + } + return std::make_shared(std::move(index)); + }, + })); + + ICEBERG_THROW_NOT_OK( + writer->Delete("data.parquet", 0, partition_spec_, PartitionValues{})); + ICEBERG_THROW_NOT_OK(writer->Close()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + // New position plus the two previous positions. + EXPECT_EQ(result.delete_files[0]->record_count, 3); + // The previous Parquet position delete is file-scoped and must be reported. + ASSERT_EQ(result.rewritten_delete_files.size(), 1u); + EXPECT_EQ(result.rewritten_delete_files[0], previous); } TEST_F(DeleteLoaderTest, LoadPositionDeletesRejectsWrongContent) { diff --git a/src/iceberg/test/deletion_vector_writer_test.cc b/src/iceberg/test/deletion_vector_writer_test.cc new file mode 100644 index 000000000..a4a0e1aea --- /dev/null +++ b/src/iceberg/test/deletion_vector_writer_test.cc @@ -0,0 +1,376 @@ +/* + * 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 +#include +#include + +#include + +#include "iceberg/data/delete_loader.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/deletes/roaring_position_bitmap.h" +#include "iceberg/file_format.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/mock_io.h" + +namespace iceberg { + +namespace { + +std::shared_ptr FindByReferencedFile( + const std::vector>& files, const std::string& ref) { + for (const auto& file : files) { + if (file->referenced_data_file == ref) { + return file; + } + } + return nullptr; +} + +std::shared_ptr UnpartitionedSpec() { + return PartitionSpec::Unpartitioned(); +} + +// A load_previous_deletes callback for data files that have no existing deletes. +Result> NoPreviousDeletes(std::string_view) { + return nullptr; +} + +} // namespace + +TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + + std::vector> delete_files; + { + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://deletes.puffin", + .io = io, + .properties = {{"created-by", "iceberg-cpp-test"}}, + .load_previous_deletes = NoPreviousDeletes, + })); + + ASSERT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 5, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-a.parquet", 10, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 1, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Delete("data-b.parquet", 2, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + delete_files = result.delete_files; + // Each referenced data file is reported once. + EXPECT_EQ(result.referenced_data_files.size(), 2u); + // No previous deletes were loaded, so nothing was rewritten. + EXPECT_TRUE(result.rewritten_delete_files.empty()); + } + + // One DataFile per referenced data file. + ASSERT_EQ(delete_files.size(), 2u); + + auto dv_a = FindByReferencedFile(delete_files, "data-a.parquet"); + auto dv_b = FindByReferencedFile(delete_files, "data-b.parquet"); + ASSERT_NE(dv_a, nullptr); + ASSERT_NE(dv_b, nullptr); + + EXPECT_EQ(dv_a->content, DataFile::Content::kPositionDeletes); + EXPECT_EQ(dv_a->file_format, FileFormatType::kPuffin); + EXPECT_TRUE(dv_a->IsDeletionVector()); + EXPECT_EQ(dv_a->file_path, "memory://deletes.puffin"); + EXPECT_EQ(dv_a->record_count, 3); + EXPECT_TRUE(dv_a->content_offset.has_value()); + EXPECT_TRUE(dv_a->content_size_in_bytes.has_value()); + EXPECT_GT(dv_a->file_size_in_bytes, 0); + EXPECT_EQ(dv_a->partition_spec_id, spec->spec_id()); + EXPECT_EQ(dv_b->record_count, 2); + + // Both blobs live in the same Puffin file but at different offsets. + EXPECT_EQ(dv_a->file_path, dv_b->file_path); + EXPECT_NE(dv_a->content_offset.value(), dv_b->content_offset.value()); + + // Read back through DeleteLoader. Each data file's DV is applied on its own + // (a single DV replaces all other deletes for that file), so the loader is + // called with just the DV that references the requested data file. + DeleteLoader loader(io); + { + auto result = loader.LoadPositionDeletes({&dv_a, 1}, "data-a.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 3); + EXPECT_TRUE(index.IsDeleted(0)); + EXPECT_TRUE(index.IsDeleted(5)); + EXPECT_TRUE(index.IsDeleted(10)); + EXPECT_FALSE(index.IsDeleted(1)); + } + + { + auto result = loader.LoadPositionDeletes({&dv_b, 1}, "data-b.parquet"); + ASSERT_THAT(result, IsOk()); + auto& index = result.value(); + EXPECT_EQ(index.Cardinality(), 2); + EXPECT_TRUE(index.IsDeleted(1)); + EXPECT_TRUE(index.IsDeleted(2)); + EXPECT_FALSE(index.IsDeleted(0)); + } +} + +// The PositionDeleteIndex overload bulk-adds positions for a data file. +TEST(DeletionVectorWriterTest, DeleteFromIndex) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + + PositionDeleteIndex positions; + positions.Delete(0); + positions.Delete(3, 6); // [3, 6) -> 3, 4, 5 + + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://from-index.puffin", + .io = io, + .load_previous_deletes = NoPreviousDeletes})); + ASSERT_THAT(writer->Delete("data.parquet", positions, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + EXPECT_EQ(result.delete_files[0]->record_count, 4); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 4); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(5)); + EXPECT_FALSE(loaded.value().IsDeleted(6)); +} + +// Previously written deletes are merged into the new vector, and the file-scoped +// delete files they came from are reported as rewritten. +TEST(DeletionVectorWriterTest, LoadPreviousDeletesMergesAndReportsRewritten) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + + // Build a previous DV index carrying its source (file-scoped) delete file, + // the same way DeleteLoader would produce it. + PositionDeleteIndex previous_positions; + previous_positions.Delete(100); + previous_positions.Delete(200); + ICEBERG_UNWRAP_OR_FAIL(auto previous_blob, previous_positions.Serialize()); + auto previous_dv = std::make_shared(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://old.puffin", + .file_format = FileFormatType::kPuffin, + .record_count = 2, + .referenced_data_file = "data.parquet", + .content_offset = 0, + .content_size_in_bytes = static_cast(previous_blob.size()), + }); + + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://merged.puffin", + .io = io, + .load_previous_deletes = + [&](std::string_view path) -> Result> { + if (path != "data.parquet") { + return nullptr; + } + ICEBERG_ASSIGN_OR_RAISE( + auto index, PositionDeleteIndex::Deserialize(previous_blob, previous_dv)); + return std::make_shared(std::move(index)); + }, + })); + + ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + // New position plus the two previous positions. + EXPECT_EQ(result.delete_files[0]->record_count, 3); + // The previous DV is file-scoped, so it is reported for removal. + ASSERT_EQ(result.rewritten_delete_files.size(), 1u); + EXPECT_EQ(result.rewritten_delete_files[0]->file_path, "memory://old.puffin"); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 3); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(100)); + EXPECT_TRUE(loaded.value().IsDeleted(200)); +} + +// A previous delete that is not file-scoped (e.g. a partition-scoped position +// delete) is merged into the new vector but is NOT reported as rewritten. +TEST(DeletionVectorWriterTest, PartitionScopedPreviousDeleteMergesButNotRewritten) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + + // A partition-scoped Parquet position delete: no referenced_data_file, so it + // is not file-scoped and must not be reported as rewritten. + auto previous_position_delete = std::make_shared(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://partition-deletes.parquet", + .file_format = FileFormatType::kParquet, + .record_count = 1, + }); + + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://merged-partition.puffin", + .io = io, + .load_previous_deletes = + [&](std::string_view path) -> Result> { + // Model what DeleteLoader produces from a Parquet position delete: + // positions plus the source delete file, not a DV blob. + auto index = std::make_shared(); + index->Delete(50); + index->AddDeleteFile(previous_position_delete); + return index; + }, + })); + + ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + ASSERT_EQ(result.delete_files.size(), 1u); + // The previous position was merged in. + EXPECT_EQ(result.delete_files[0]->record_count, 2); + // The previous delete is partition-scoped, so it is not rewritten. + EXPECT_TRUE(result.rewritten_delete_files.empty()); + + DeleteLoader loader(io); + auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + ASSERT_THAT(loaded, IsOk()); + EXPECT_EQ(loaded.value().Cardinality(), 2); + EXPECT_TRUE(loaded.value().IsDeleted(0)); + EXPECT_TRUE(loaded.value().IsDeleted(50)); +} + +TEST(DeletionVectorWriterTest, EmptyWriterProducesNoDataFiles) { + auto io = std::make_shared(); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://empty.puffin", + .io = io, + .load_previous_deletes = NoPreviousDeletes})); + ASSERT_THAT(writer->Close(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); + EXPECT_TRUE(result.delete_files.empty()); + + // No blobs were written, so no (orphan) Puffin file should have been created. + EXPECT_THAT(io->NewInputFile("memory://empty.puffin"), IsError(ErrorKind::kNotFound)); +} + +TEST(DeletionVectorWriterTest, DeleteRejectsEmptyReferencedFile) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://invalid.puffin", + .io = io, + .load_previous_deletes = NoPreviousDeletes})); + EXPECT_THAT(writer->Delete("", 0, spec, PartitionValues{}), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(DeletionVectorWriterTest, DeleteRejectsOutOfRangePosition) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://invalid.puffin", + .io = io, + .load_previous_deletes = NoPreviousDeletes})); + // Negative and out-of-range positions are rejected rather than silently + // dropped by the underlying bitmap. + EXPECT_THAT(writer->Delete("data.parquet", -1, spec, PartitionValues{}), + IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(writer->Delete("data.parquet", RoaringPositionBitmap::kMaxPosition + 1, + spec, PartitionValues{}), + IsError(ErrorKind::kInvalidArgument)); +} + +// Close propagates a load_previous_deletes failure and returns no metadata. +TEST(DeletionVectorWriterTest, ClosePropagatesLoadPreviousDeletesError) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + ICEBERG_UNWRAP_OR_FAIL( + auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://err.puffin", + .io = io, + .load_previous_deletes = [](std::string_view) + -> Result> { return IOError("boom"); }, + })); + + ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); + EXPECT_THAT(writer->Close(), IsError(ErrorKind::kIOError)); + // No partial delete metadata is exposed after a failed Close. + EXPECT_THAT(writer->Metadata(), IsError(ErrorKind::kValidationFailed)); + // No orphan Puffin file was created. + EXPECT_THAT(io->NewInputFile("memory://err.puffin"), IsError(ErrorKind::kNotFound)); +} + +TEST(DeletionVectorWriterTest, MakeRejectsNullIo) { + EXPECT_THAT(DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "x.puffin", .load_previous_deletes = NoPreviousDeletes}), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(DeletionVectorWriterTest, MakeRejectsEmptyPath) { + auto io = std::make_shared(); + EXPECT_THAT(DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .io = io, .load_previous_deletes = NoPreviousDeletes}), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(DeletionVectorWriterTest, MakeRejectsMissingLoadPreviousDeletes) { + auto io = std::make_shared(); + EXPECT_THAT(DeletionVectorWriter::Make( + DeletionVectorWriterOptions{.path = "x.puffin", .io = io}), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(DeletionVectorWriterTest, DeleteAfterCloseFails) { + auto io = std::make_shared(); + auto spec = UnpartitionedSpec(); + ICEBERG_UNWRAP_OR_FAIL(auto writer, + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "memory://closed.puffin", + .io = io, + .load_previous_deletes = NoPreviousDeletes})); + ASSERT_THAT(writer->Close(), IsOk()); + EXPECT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), + IsError(ErrorKind::kValidationFailed)); +} + +} // namespace iceberg diff --git a/src/iceberg/test/file_scan_task_reader_test.cc b/src/iceberg/test/file_scan_task_reader_test.cc index 1630a108a..59af21a93 100644 --- a/src/iceberg/test/file_scan_task_reader_test.cc +++ b/src/iceberg/test/file_scan_task_reader_test.cc @@ -38,6 +38,7 @@ #include "iceberg/arrow/arrow_register.h" #include "iceberg/arrow_c_data_guard_internal.h" #include "iceberg/arrow_c_data_util_internal.h" +#include "iceberg/data/deletion_vector_writer.h" #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" #include "iceberg/file_format.h" @@ -298,6 +299,25 @@ class FileScanTaskReaderTest : public TempFileTestBase { return metadata.data_files[0]; } + Result> MakeDeletionVectorFile( + const std::string& path, const std::vector& positions, + const std::string& data_path) { + DeletionVectorWriterOptions options{ + .path = path, + .io = file_io_, + .load_previous_deletes = [](std::string_view) + -> Result> { return nullptr; }, + }; + ICEBERG_ASSIGN_OR_RAISE(auto writer, DeletionVectorWriter::Make(std::move(options))); + for (int64_t pos : positions) { + ICEBERG_RETURN_UNEXPECTED( + writer->Delete(data_path, pos, partition_spec_, PartitionValues{})); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + ICEBERG_ASSIGN_OR_RAISE(auto metadata, writer->Metadata()); + return metadata.delete_files[0]; + } + void VerifyStream(struct ArrowArrayStream* stream, std::string_view expected_json) { auto record_batch_reader = ::arrow::ImportRecordBatchReader(stream).ValueOrDie(); @@ -401,6 +421,30 @@ TEST_F(FileScanTaskReaderTest, OpenWithPositionDeletesFiltersRowsAndPrunesPos) { ASSERT_NO_FATAL_FAILURE(VerifyStream(&stream, R"([[1, "Foo"], [3, "Baz"]])")); } +TEST_F(FileScanTaskReaderTest, OpenWithDeletionVectorFiltersRows) { + ICEBERG_UNWRAP_OR_FAIL( + auto data_file, + MakeDataFile(table_schema_, + R"([[1, "Foo", "blue"], [2, "Bar", "red"], [3, "Baz", "green"]])")); + ICEBERG_UNWRAP_OR_FAIL( + auto dv, MakeDeletionVectorFile(CreateNewTempFilePathWithSuffix(".puffin"), {1}, + data_file->file_path)); + FileScanTask task(data_file, {dv}); + + FileScanTaskReader::Options options{ + .io = file_io_, + .table_schema = table_schema_, + .schemas = {table_schema_}, + .projected_schema = projected_schema_, + }; + ICEBERG_UNWRAP_OR_FAIL(auto reader, FileScanTaskReader::Make(std::move(options))); + auto stream_result = reader->Open(task); + ASSERT_THAT(stream_result, IsOk()); + auto stream = std::move(stream_result.value()); + + ASSERT_NO_FATAL_FAILURE(VerifyStream(&stream, R"([[1, "Foo"], [3, "Baz"]])")); +} + TEST_F(FileScanTaskReaderTest, OpenWithEqualityDeletesAddsAndPrunesDeleteOnlyColumns) { ICEBERG_UNWRAP_OR_FAIL( auto data_file, diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 12460237d..2e07d273c 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -130,6 +130,10 @@ iceberg_tests = { ), 'use_data': true, }, + 'deletion_vector_writer_test': { + 'sources': files('deletion_vector_writer_test.cc'), + 'use_data': true, + }, } if get_option('rest').enabled() diff --git a/src/iceberg/test/position_delete_index_test.cc b/src/iceberg/test/position_delete_index_test.cc index 0c24a587b..124f06224 100644 --- a/src/iceberg/test/position_delete_index_test.cc +++ b/src/iceberg/test/position_delete_index_test.cc @@ -19,10 +19,36 @@ #include "iceberg/deletes/position_delete_index.h" +#include +#include +#include + #include +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/test/matchers.h" + namespace iceberg { +namespace { + +// A source delete file matching a serialized blob: content_size_in_bytes and +// record_count are what Deserialize validates against. +std::shared_ptr DeleteFileFor(const std::vector& blob, + int64_t record_count) { + return std::make_shared(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = "memory://dv.puffin", + .file_format = FileFormatType::kPuffin, + .record_count = record_count, + .referenced_data_file = "data.parquet", + .content_offset = 0, + .content_size_in_bytes = static_cast(blob.size()), + }); +} + +} // namespace + TEST(PositionDeleteIndexTest, TestEmptyIndex) { PositionDeleteIndex index; ASSERT_TRUE(index.IsEmpty()); @@ -200,4 +226,127 @@ TEST(PositionDeleteIndexTest, TestMergeIdempotence) { ASSERT_TRUE(index1.IsDeleted(20)); } +// ==================== deletion-vector-v1 serialization ==================== + +TEST(PositionDeleteIndexTest, SerializeRoundTrip) { + PositionDeleteIndex index; + for (int64_t pos : + {int64_t{0}, int64_t{1}, int64_t{5}, int64_t{100}, int64_t{4'000'000'000}}) { + index.Delete(pos); + } + + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, + PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 5))); + + EXPECT_EQ(restored.Cardinality(), 5); + for (int64_t pos : + {int64_t{0}, int64_t{1}, int64_t{5}, int64_t{100}, int64_t{4'000'000'000}}) { + EXPECT_TRUE(restored.IsDeleted(pos)); + } +} + +TEST(PositionDeleteIndexTest, SerializeEmptyRoundTrip) { + PositionDeleteIndex index; + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, + PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 0))); + EXPECT_TRUE(restored.IsEmpty()); +} + +// Spans two high-32-bit keys and exercises all Roaring container types +// (sparse "array", dense "bitset", and run containers after optimization). +TEST(PositionDeleteIndexTest, SerializeAllContainerTypesAcrossKeys) { + constexpr int64_t kKeyStride = 0x100000000LL; // 2^32: high-32-bit key + constexpr int64_t kContainerStride = 1 << 16; // 2^16: Roaring container + auto pos = [](int64_t key, int64_t container, int64_t value) { + return key * kKeyStride + container * kContainerStride + value; + }; + + PositionDeleteIndex index; + int64_t expected = 0; + for (int64_t key : {int64_t{0}, int64_t{1}}) { + index.Delete(pos(key, 0, 5)); + index.Delete(pos(key, 0, 7)); + expected += 2; + index.Delete(pos(key, 1, 1), pos(key, 1, 1000)); + expected += 999; + index.Delete(pos(key, 2, 1), pos(key, 2, kContainerStride)); + expected += kContainerStride - 1; + } + + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto restored, PositionDeleteIndex::Deserialize( + blob, DeleteFileFor(blob, expected))); + + EXPECT_EQ(restored.Cardinality(), expected); + EXPECT_TRUE(restored.IsDeleted(pos(0, 0, 5))); + EXPECT_TRUE(restored.IsDeleted(pos(1, 2, kContainerStride - 1))); + EXPECT_TRUE(restored.IsDeleted(pos(0, 1, 999))); + EXPECT_FALSE(restored.IsDeleted(pos(0, 0, 6))); + EXPECT_FALSE(restored.IsDeleted(pos(1, 1, 1000))); // range end is exclusive +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsCorruptedCrc) { + PositionDeleteIndex index; + index.Delete(1); + index.Delete(2); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + blob.back() ^= 0xFF; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 2)), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsBadMagic) { + PositionDeleteIndex index; + index.Delete(1); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + blob[4] = 0x00; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 1)), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsTruncatedBlob) { + std::vector blob = {0x00, 0x00}; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 0)), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsCardinalityMismatch) { + PositionDeleteIndex index; + index.Delete(1); + index.Delete(2); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + // record_count in metadata disagrees with the bitmap cardinality. + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob, DeleteFileFor(blob, 99)), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializeRejectsContentSizeMismatch) { + PositionDeleteIndex index; + index.Delete(1); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + auto delete_file = DeleteFileFor(blob, 1); + delete_file->content_size_in_bytes = static_cast(blob.size()) + 1; + EXPECT_THAT(PositionDeleteIndex::Deserialize(blob, delete_file), + IsError(ErrorKind::kInvalidArgument)); +} + +TEST(PositionDeleteIndexTest, DeserializePreservesSourceDeleteFile) { + PositionDeleteIndex index; + index.Delete(1); + index.Delete(2); + ICEBERG_UNWRAP_OR_FAIL(auto blob, index.Serialize()); + + auto delete_file = DeleteFileFor(blob, 2); + ICEBERG_UNWRAP_OR_FAIL(auto restored, + PositionDeleteIndex::Deserialize(blob, delete_file)); + ASSERT_EQ(restored.delete_files().size(), 1u); + EXPECT_EQ(restored.delete_files()[0], delete_file); +} + } // namespace iceberg From 680b90b3afbac3ca4a3f4870a10df298cea7df90 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 14 Jul 2026 16:08:01 +0800 Subject: [PATCH 2/3] Align deletion vector writer with Java behavior - Reuse `WriteResult` for delete writers and report referenced/rewritten delete files. - Add shared DV read helper and refactor DV loading/writing paths. - Preserve source delete files in `PositionDeleteIndex` and tighten DV serialization. - Generate Iceberg-CPP `created-by` metadata consistently in CMake and Meson. - Fold DV writer tests into existing test targets. --- CMakeLists.txt | 8 +- cmake_modules/IcebergBuildUtils.cmake | 22 ++ src/iceberg/CMakeLists.txt | 1 + src/iceberg/data/data_writer.cc | 6 +- src/iceberg/data/delete_loader.cc | 118 +++------- src/iceberg/data/delete_loader.h | 7 +- src/iceberg/data/deletion_vector_writer.cc | 218 +++++++++--------- src/iceberg/data/deletion_vector_writer.h | 34 +-- src/iceberg/data/dv_util.cc | 68 ++++++ src/iceberg/data/dv_util_internal.h | 40 ++++ src/iceberg/data/equality_delete_writer.cc | 8 +- src/iceberg/data/position_delete_writer.cc | 8 +- src/iceberg/data/writer.h | 22 +- src/iceberg/deletes/position_delete_index.cc | 168 ++++++-------- src/iceberg/deletes/position_delete_index.h | 13 +- .../deletes/roaring_position_bitmap.cc | 33 +-- src/iceberg/deletes/roaring_position_bitmap.h | 3 + src/iceberg/meson.build | 27 ++- src/iceberg/test/CMakeLists.txt | 4 +- src/iceberg/test/delete_filter_test.cc | 5 +- src/iceberg/test/delete_loader_test.cc | 44 ++-- .../test/deletion_vector_writer_test.cc | 208 +++++++++-------- src/iceberg/test/endian_test.cc | 3 + .../test/file_scan_task_reader_test.cc | 5 +- src/iceberg/test/meson.build | 5 +- .../test/position_delete_index_test.cc | 14 ++ src/iceberg/util/endian.h | 17 ++ src/iceberg/util/string_util.h | 7 + src/iceberg/version.h.in | 2 + 29 files changed, 615 insertions(+), 503 deletions(-) create mode 100644 src/iceberg/data/dv_util.cc create mode 100644 src/iceberg/data/dv_util_internal.h diff --git a/CMakeLists.txt b/CMakeLists.txt index db54b24ed..7cbd5e55e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -28,10 +28,9 @@ project(Iceberg DESCRIPTION "Iceberg C++ Project" LANGUAGES CXX) -set(ICEBERG_VERSION_SUFFIX "-SNAPSHOT") -set(ICEBERG_VERSION_STRING "${PROJECT_VERSION}${ICEBERG_VERSION_SUFFIX}") -configure_file("${CMAKE_CURRENT_SOURCE_DIR}/src/iceberg/version.h.in" - "${CMAKE_BINARY_DIR}/src/iceberg/version.h") +include(IcebergBuildUtils) +iceberg_configure_version_header("${CMAKE_CURRENT_SOURCE_DIR}/src/iceberg/version.h.in" + "${CMAKE_BINARY_DIR}/src/iceberg/version.h") set(CMAKE_CXX_STANDARD 23) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -79,7 +78,6 @@ if(ICEBERG_BUILD_REST_INTEGRATION_TESTS AND WIN32) endif() include(CMakeParseArguments) -include(IcebergBuildUtils) include(IcebergSanitizer) include(IcebergSccache) include(IcebergThirdpartyToolchain) diff --git a/cmake_modules/IcebergBuildUtils.cmake b/cmake_modules/IcebergBuildUtils.cmake index da23f37a2..689ec44b7 100644 --- a/cmake_modules/IcebergBuildUtils.cmake +++ b/cmake_modules/IcebergBuildUtils.cmake @@ -20,6 +20,28 @@ include(CMakePackageConfigHelpers) +function(iceberg_configure_version_header INPUT_FILE OUTPUT_FILE) + set(ICEBERG_VERSION_SUFFIX "-SNAPSHOT") + set(ICEBERG_VERSION_STRING "${PROJECT_VERSION}${ICEBERG_VERSION_SUFFIX}") + set(ICEBERG_GIT_COMMIT_ID "unknown") + + find_package(Git QUIET) + if(GIT_FOUND) + execute_process(COMMAND "${GIT_EXECUTABLE}" rev-parse HEAD + WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}" + OUTPUT_VARIABLE ICEBERG_GIT_COMMIT_ID + RESULT_VARIABLE ICEBERG_GIT_RESULT + OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET) + if(NOT ICEBERG_GIT_RESULT EQUAL 0 OR NOT ICEBERG_GIT_COMMIT_ID) + set(ICEBERG_GIT_COMMIT_ID "unknown") + endif() + endif() + + set(ICEBERG_FULL_VERSION_STRING + "Apache Iceberg-CPP ${ICEBERG_VERSION_STRING} (commit ${ICEBERG_GIT_COMMIT_ID})") + configure_file("${INPUT_FILE}" "${OUTPUT_FILE}" @ONLY) +endfunction() + function(iceberg_install_cmake_package PACKAGE_NAME EXPORT_NAME) set(CONFIG_CMAKE "${PACKAGE_NAME}-config.cmake") set(BUILT_CONFIG_CMAKE "${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_CMAKE}") diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index d8787892e..35b7b91b5 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -197,6 +197,7 @@ set(ICEBERG_DATA_SOURCES data/delete_filter.cc data/delete_loader.cc data/deletion_vector_writer.cc + data/dv_util.cc data/equality_delete_writer.cc data/file_scan_task_reader.cc data/position_delete_writer.cc diff --git a/src/iceberg/data/data_writer.cc b/src/iceberg/data/data_writer.cc index 420471280..a63f05530 100644 --- a/src/iceberg/data/data_writer.cc +++ b/src/iceberg/data/data_writer.cc @@ -58,7 +58,7 @@ class DataWriter::Impl { return {}; } - Result Metadata() { + Result Metadata() { ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer"); ICEBERG_ASSIGN_OR_RAISE(auto metrics, writer_->metrics()); @@ -98,7 +98,7 @@ class DataWriter::Impl { options_.spec ? std::make_optional(options_.spec->spec_id()) : std::nullopt, }); - FileWriter::WriteResult result; + WriteResult result; result.data_files.push_back(std::move(data_file)); return result; } @@ -127,6 +127,6 @@ Result DataWriter::Length() const { return impl_->Length(); } Status DataWriter::Close() { return impl_->Close(); } -Result DataWriter::Metadata() { return impl_->Metadata(); } +Result DataWriter::Metadata() { return impl_->Metadata(); } } // namespace iceberg diff --git a/src/iceberg/data/delete_loader.cc b/src/iceberg/data/delete_loader.cc index ad9207f5e..a95fab7e6 100644 --- a/src/iceberg/data/delete_loader.cc +++ b/src/iceberg/data/delete_loader.cc @@ -31,6 +31,7 @@ #include "iceberg/arrow/nanoarrow_status_internal.h" #include "iceberg/arrow_c_data_guard_internal.h" +#include "iceberg/data/dv_util_internal.h" #include "iceberg/deletes/position_delete_index.h" #include "iceberg/deletes/position_delete_range_consumer.h" #include "iceberg/file_io.h" @@ -88,17 +89,31 @@ bool StringEquals(const ArrowArrayView* view, int64_t row_idx, std::string_view return sv.data != nullptr && std::memcmp(sv.data, target.data(), target.size()) == 0; } +Status ValidateDV(const DataFile& dv, std::string_view data_file_path) { + ICEBERG_PRECHECK(dv.content_offset.has_value(), "Invalid DV, offset cannot be null: {}", + ContentFileUtil::DVDesc(dv)); + ICEBERG_PRECHECK(dv.content_size_in_bytes.has_value(), "Invalid DV, length is null: {}", + ContentFileUtil::DVDesc(dv)); + ICEBERG_PRECHECK( + dv.content_size_in_bytes.value() <= std::numeric_limits::max(), + "Can't read DV larger than 2GB: {}", dv.content_size_in_bytes.value()); + ICEBERG_PRECHECK(dv.referenced_data_file.has_value() && + dv.referenced_data_file.value() == data_file_path, + "DV is expected to reference {}, not {}", data_file_path, + dv.referenced_data_file.value_or("")); + return {}; +} + } // namespace DeleteLoader::DeleteLoader(std::shared_ptr io) : io_(std::move(io)) {} DeleteLoader::~DeleteLoader() = default; -Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, - PositionDeleteIndex& index, +Status DeleteLoader::LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index, std::string_view data_file_path) const { - // TODO(gangwu): push down path filter to open the file. - ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(*file, PosDeleteSchema(), io_)); + // TODO(gangwu): Add cache hooks, worker pool, and filter pushdown. + ICEBERG_ASSIGN_OR_RAISE(auto reader, OpenDeleteFile(file, PosDeleteSchema(), io_)); ICEBERG_ASSIGN_OR_RAISE(auto arrow_schema, reader->Schema()); internal::ArrowSchemaGuard schema_guard(&arrow_schema); @@ -116,8 +131,8 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, // `ForEachPositionDelete`. Trusts the hint -- spec-compliant writers // only set it when all rows share one data file. const bool use_referenced_data_file_fast_path = - file->referenced_data_file.has_value() && - file->referenced_data_file.value() == data_file_path; + file.referenced_data_file.has_value() && + file.referenced_data_file.value() == data_file_path; // Filter-path staging buffer; reused across batches via `clear()`. std::vector positions; @@ -125,10 +140,6 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, // reused across batches and across both routing branches. std::vector bulk_scratch; - // Whether any position for the target data file came from this file, so the - // source delete file is recorded once. - bool contributed = false; - while (true) { ICEBERG_ASSIGN_OR_RAISE(auto batch_opt, reader->Next()); if (!batch_opt.has_value()) break; @@ -160,9 +171,6 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, const int64_t* pos_data = Int64ValuesBuffer(pos_view); if (use_referenced_data_file_fast_path) { - // Reaching here means length > 0 (empty batches are skipped above), so - // this file contributes positions for the target data file. - contributed = true; ForEachPositionDelete(std::span(pos_data, length), index, bulk_scratch); continue; @@ -177,67 +185,10 @@ Status DeleteLoader::LoadPositionDelete(const std::shared_ptr& file, positions.push_back(pos_data[i]); } } - if (!positions.empty()) { - contributed = true; - } ForEachPositionDelete(positions, index, bulk_scratch); } - ICEBERG_RETURN_UNEXPECTED(reader->Close()); - - // Record the source delete file so callers (e.g. DeletionVectorWriter) can - // report file-scoped position deletes as rewritten. - if (contributed) { - index.AddDeleteFile(file); - } - return {}; -} - -Status DeleteLoader::LoadDV(const std::shared_ptr& file, - PositionDeleteIndex& index, - std::string_view data_file_path) const { - // A deletion vector must reference exactly one data file; without it the - // caller cannot know which data file the positions apply to. - ICEBERG_PRECHECK(file->referenced_data_file.has_value(), - "Deletion vector requires referenced_data_file: {}", file->file_path); - - // The DV must reference the data file being read. A mismatch means the caller - // grouped delete files incorrectly; failing here avoids silently applying the - // wrong DV or hiding bad metadata behind an empty index. - ICEBERG_PRECHECK(file->referenced_data_file.value() == data_file_path, - "Deletion vector references {}, not the requested data file {}", - file->referenced_data_file.value(), data_file_path); - - // For deletion vectors, content_offset and content_size_in_bytes point directly - // at the DV blob bytes within the Puffin file and are required by the spec. - ICEBERG_PRECHECK( - file->content_offset.has_value() && file->content_size_in_bytes.has_value(), - "Deletion vector requires content_offset and content_size_in_bytes: {}", - file->file_path); - - const int64_t offset = file->content_offset.value(); - const int64_t length = file->content_size_in_bytes.value(); - ICEBERG_PRECHECK(offset >= 0 && length >= 0, - "Invalid deletion vector offset/length: offset={}, length={}", offset, - length); - ICEBERG_PRECHECK(length <= std::numeric_limits::max(), - "Cannot read deletion vector larger than 2GB: {}", length); - - ICEBERG_ASSIGN_OR_RAISE(auto input_file, io_->NewInputFile(file->file_path)); - ICEBERG_ASSIGN_OR_RAISE(auto stream, input_file->Open()); - - std::vector bytes(static_cast(length)); - ICEBERG_RETURN_UNEXPECTED(stream->ReadFully(offset, bytes)); - ICEBERG_RETURN_UNEXPECTED(stream->Close()); - - std::span blob(reinterpret_cast(bytes.data()), - bytes.size()); - // Deserialize validates the blob length and cardinality against `file` and - // retains it as the source delete file. - ICEBERG_ASSIGN_OR_RAISE(auto dv, PositionDeleteIndex::Deserialize(blob, file)); - - index.Merge(dv); - return {}; + return reader->Close(); } Result DeleteLoader::LoadPositionDeletes( @@ -247,19 +198,14 @@ Result DeleteLoader::LoadPositionDeletes( ICEBERG_PRECHECK(file != nullptr, "Delete file must not be null"); } - PositionDeleteIndex index; - - // A single deletion vector replaces all other deletes for a data file, so it - // is read through the dedicated DV path and validated against the requested - // data file. if (ContentFileUtil::ContainsSingleDV(delete_files)) { - ICEBERG_RETURN_UNEXPECTED(LoadDV(delete_files.front(), index, data_file_path)); - return index; + const auto& dv = delete_files.front(); + ICEBERG_RETURN_UNEXPECTED(ValidateDV(*dv, data_file_path)); + return DVUtil::ReadDV(dv, io_); } - // Otherwise all entries must be position delete files. A DV must not be mixed - // with position deletes: once a DV applies, readers ignore matching position - // deletes, so a mixed list means the caller grouped delete files incorrectly. + std::vector> position_delete_files; + for (const auto& file : delete_files) { ICEBERG_PRECHECK(!file->IsDeletionVector(), "Deletion vector cannot be mixed with position delete files: {}", @@ -268,14 +214,18 @@ Result DeleteLoader::LoadPositionDeletes( "Expected position delete file but got content type {}", ToString(file->content)); - // A file-scoped position delete for another data file has nothing for the - // target path; skip it as an optimization. if (file->referenced_data_file.has_value() && file->referenced_data_file.value() != data_file_path) { continue; } - ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(file, index, data_file_path)); + position_delete_files.push_back(file); + } + + PositionDeleteIndex index(position_delete_files); + + for (const auto& file : position_delete_files) { + ICEBERG_RETURN_UNEXPECTED(LoadPositionDelete(*file, index, data_file_path)); } return index; diff --git a/src/iceberg/data/delete_loader.h b/src/iceberg/data/delete_loader.h index 6bd07fd8e..360914e85 100644 --- a/src/iceberg/data/delete_loader.h +++ b/src/iceberg/data/delete_loader.h @@ -67,14 +67,9 @@ class ICEBERG_DATA_EXPORT DeleteLoader { private: /// \brief Load a single position delete file into the index. - Status LoadPositionDelete(const std::shared_ptr& file, - PositionDeleteIndex& index, + Status LoadPositionDelete(const DataFile& file, PositionDeleteIndex& index, std::string_view data_file_path) const; - /// \brief Load a single deletion vector file into the index. - Status LoadDV(const std::shared_ptr& file, PositionDeleteIndex& index, - std::string_view data_file_path) const; - std::shared_ptr io_; }; diff --git a/src/iceberg/data/deletion_vector_writer.cc b/src/iceberg/data/deletion_vector_writer.cc index d4a9ca5ea..27a84a022 100644 --- a/src/iceberg/data/deletion_vector_writer.cc +++ b/src/iceberg/data/deletion_vector_writer.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -34,6 +35,7 @@ #include "iceberg/puffin/puffin_writer.h" #include "iceberg/util/content_file_util.h" #include "iceberg/util/macros.h" +#include "iceberg/util/string_util.h" #include "iceberg/version.h" namespace iceberg { @@ -55,36 +57,42 @@ class DeletionVectorWriter::Impl { }; Deletes& DeletesFor(std::string_view referenced_data_file, - std::shared_ptr spec, PartitionValues partition) { - auto [it, inserted] = deletes_by_path_.try_emplace(std::string(referenced_data_file)); - if (inserted) { - it->second.spec = std::move(spec); - it->second.partition = std::move(partition); + const std::shared_ptr& spec, + const PartitionValues& partition) { + auto it = deletes_by_path_.lower_bound(referenced_data_file); + if (it == deletes_by_path_.end() || + deletes_by_path_.key_comp()(referenced_data_file, it->first)) { + it = + deletes_by_path_.emplace_hint(it, std::string(referenced_data_file), Deletes{}); + it->second.spec = spec; + it->second.partition = partition; } return it->second; } Status Delete(std::string_view referenced_data_file, int64_t pos, - std::shared_ptr spec, PartitionValues partition) { - ICEBERG_CHECK(!closed_, "Cannot delete after the writer is closed"); + const std::shared_ptr& spec, + const PartitionValues& partition) { + ICEBERG_CHECK(!closed_, "Cannot delete after writer is closed"); ICEBERG_PRECHECK(!referenced_data_file.empty(), "Deletion vector requires a non-empty referenced data file"); + ICEBERG_PRECHECK(spec != nullptr, "Deletion vector requires a partition spec"); ICEBERG_PRECHECK(pos >= 0 && pos <= RoaringPositionBitmap::kMaxPosition, "Deletion vector position out of range [0, {}]: {}", RoaringPositionBitmap::kMaxPosition, pos); - DeletesFor(referenced_data_file, std::move(spec), std::move(partition)) - .positions.Delete(pos); + DeletesFor(referenced_data_file, spec, partition).positions.Delete(pos); return {}; } Status Delete(std::string_view referenced_data_file, - const PositionDeleteIndex& positions, std::shared_ptr spec, - PartitionValues partition) { - ICEBERG_CHECK(!closed_, "Cannot delete after the writer is closed"); + const PositionDeleteIndex& positions, + const std::shared_ptr& spec, + const PartitionValues& partition) { + ICEBERG_CHECK(!closed_, "Cannot delete after writer is closed"); ICEBERG_PRECHECK(!referenced_data_file.empty(), "Deletion vector requires a non-empty referenced data file"); - DeletesFor(referenced_data_file, std::move(spec), std::move(partition)) - .positions.Merge(positions); + ICEBERG_PRECHECK(spec != nullptr, "Deletion vector requires a partition spec"); + DeletesFor(referenced_data_file, spec, partition).positions.Merge(positions); return {}; } @@ -93,115 +101,114 @@ class DeletionVectorWriter::Impl { return {}; } - // No deletes: skip creating an orphan Puffin file that no metadata - // references. if (deletes_by_path_.empty()) { closed_ = true; return {}; } - // Merge previously written deletes and collect the file-scoped delete files - // they came from so callers can remove them from table state. for (auto& [path, deletes] : deletes_by_path_) { - ICEBERG_ASSIGN_OR_RAISE(auto previous, options_.load_previous_deletes(path)); - if (previous == nullptr) { - continue; - } - deletes.positions.Merge(*previous); - for (const auto& delete_file : previous->delete_files()) { - ICEBERG_ASSIGN_OR_RAISE(bool file_scoped, - ContentFileUtil::IsFileScoped(*delete_file)); - if (file_scoped) { - result_.rewritten_delete_files.push_back(delete_file); - } - } - } - - auto properties = options_.properties; - if (const std::string created_by(puffin::StandardPuffinProperties::kCreatedBy); - !properties.contains(created_by)) { - properties.emplace(created_by, - std::format("iceberg-cpp/{}.{}.{}", ICEBERG_VERSION_MAJOR, - ICEBERG_VERSION_MINOR, ICEBERG_VERSION_PATCH)); + ICEBERG_RETURN_UNEXPECTED(LoadPreviousDeletes(path, deletes)); } ICEBERG_ASSIGN_OR_RAISE(auto output_file, options_.io->NewOutputFile(options_.path)); + const std::string output_path(options_.path); ICEBERG_ASSIGN_OR_RAISE( - auto writer, - puffin::PuffinWriter::Make(std::move(output_file), std::move(properties))); - - struct Entry { - std::string referenced_data_file; - std::shared_ptr spec; - PartitionValues partition; - int64_t offset; - int64_t length; - int64_t cardinality; - }; - std::vector entries; - entries.reserve(deletes_by_path_.size()); + auto writer, puffin::PuffinWriter::Make( + std::move(output_file), + {{std::string(puffin::StandardPuffinProperties::kCreatedBy), + ICEBERG_FULL_VERSION_STRING}})); for (auto& [path, deletes] : deletes_by_path_) { - const int64_t cardinality = deletes.positions.Cardinality(); - ICEBERG_ASSIGN_OR_RAISE(auto data, deletes.positions.Serialize()); - - puffin::Blob blob{ - .type = std::string(puffin::StandardBlobTypes::kDeletionVectorV1), - .input_fields = {MetadataColumns::kFilePositionColumnId}, - // Snapshot ID and sequence number are inherited; the spec requires -1. - .snapshot_id = -1, - .sequence_number = -1, - .data = std::move(data), - .requested_compression = puffin::PuffinCompressionCodec::kNone, - }; - blob.properties.emplace(std::string(kReferencedDataFile), path); - blob.properties.emplace(std::string(kCardinality), std::format("{}", cardinality)); - - ICEBERG_ASSIGN_OR_RAISE(auto blob_metadata, writer->Write(blob)); - entries.push_back(Entry{ - .referenced_data_file = path, - .spec = deletes.spec, - .partition = deletes.partition, - .offset = blob_metadata.offset, - .length = blob_metadata.length, - .cardinality = cardinality, - }); + ICEBERG_RETURN_UNEXPECTED(Write(*writer, path, deletes)); } ICEBERG_RETURN_UNEXPECTED(writer->Finish()); ICEBERG_ASSIGN_OR_RAISE(const int64_t file_size, writer->FileSize()); - for (auto& entry : entries) { - result_.referenced_data_files.push_back(entry.referenced_data_file); - result_.delete_files.push_back(std::make_shared(DataFile{ - .content = DataFile::Content::kPositionDeletes, - .file_path = options_.path, - .file_format = FileFormatType::kPuffin, - .partition = std::move(entry.partition), - .record_count = entry.cardinality, - .file_size_in_bytes = file_size, - .referenced_data_file = std::move(entry.referenced_data_file), - .content_offset = entry.offset, - .content_size_in_bytes = entry.length, - .partition_spec_id = - entry.spec ? std::make_optional(entry.spec->spec_id()) : std::nullopt, - })); + for (const auto& [path, _] : deletes_by_path_) { + result_.referenced_data_files.push_back(path); + ICEBERG_ASSIGN_OR_RAISE(auto data_file, CreateDV(output_path, file_size, path)); + result_.data_files.push_back(std::move(data_file)); } closed_ = true; return {}; } - Result Metadata() { + Result Metadata() { ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer"); return result_; } private: + Status LoadPreviousDeletes(std::string_view path, Deletes& deletes) { + ICEBERG_ASSIGN_OR_RAISE(auto previous, options_.load_previous_deletes(path)); + if (!previous.has_value()) { + return {}; + } + + deletes.positions.Merge(*previous); + for (const auto& delete_file : previous->delete_files()) { + ICEBERG_ASSIGN_OR_RAISE(bool file_scoped, + ContentFileUtil::IsFileScoped(*delete_file)); + if (file_scoped) { + result_.rewritten_delete_files.push_back(delete_file); + } + } + return {}; + } + + Status Write(puffin::PuffinWriter& writer, std::string_view path, Deletes& deletes) { + const int64_t cardinality = deletes.positions.Cardinality(); + ICEBERG_ASSIGN_OR_RAISE(auto data, deletes.positions.Serialize()); + + puffin::Blob blob{ + .type = std::string(puffin::StandardBlobTypes::kDeletionVectorV1), + .input_fields = {MetadataColumns::kFilePositionColumnId}, + // Snapshot ID and sequence number are inherited; the spec requires -1. + .snapshot_id = -1, + .sequence_number = -1, + .data = std::move(data), + .requested_compression = puffin::PuffinCompressionCodec::kNone, + }; + blob.properties.emplace(std::string(kReferencedDataFile), path); + blob.properties.emplace(std::string(kCardinality), std::format("{}", cardinality)); + + ICEBERG_ASSIGN_OR_RAISE(auto blob_metadata, writer.Write(blob)); + blobs_by_path_.insert_or_assign(std::string(path), std::move(blob_metadata)); + return {}; + } + + Result> CreateDV(const std::string& path, int64_t size, + std::string_view referenced_data_file) { + auto deletes = deletes_by_path_.find(referenced_data_file); + ICEBERG_CHECK(deletes != deletes_by_path_.end(), "Missing deletion vector for {}", + referenced_data_file); + auto blob_metadata = blobs_by_path_.find(referenced_data_file); + ICEBERG_CHECK(blob_metadata != blobs_by_path_.end(), + "Missing deletion vector blob for {}", referenced_data_file); + + return std::make_shared(DataFile{ + .content = DataFile::Content::kPositionDeletes, + .file_path = path, + .file_format = FileFormatType::kPuffin, + .partition = deletes->second.partition, + .record_count = deletes->second.positions.Cardinality(), + .file_size_in_bytes = size, + // TODO(gangwu): support encryption key metadata + .referenced_data_file = std::string(referenced_data_file), + .content_offset = blob_metadata->second.offset, + .content_size_in_bytes = blob_metadata->second.length, + .partition_spec_id = deletes->second.spec + ? std::make_optional(deletes->second.spec->spec_id()) + : std::nullopt, + }); + } + DeletionVectorWriterOptions options_; - // Ordered by referenced data file path for deterministic blob layout. - std::map deletes_by_path_; - DeleteWriteResult result_; + std::map deletes_by_path_; + std::map blobs_by_path_; + WriteResult result_; bool closed_ = false; }; @@ -212,32 +219,29 @@ DeletionVectorWriter::~DeletionVectorWriter() = default; Result> DeletionVectorWriter::Make( DeletionVectorWriterOptions options) { + ICEBERG_PRECHECK(!options.path.empty(), "DeletionVectorWriter requires an output path"); ICEBERG_PRECHECK(options.io != nullptr, "DeletionVectorWriter requires a FileIO"); - ICEBERG_PRECHECK(!options.path.empty(), "DeletionVectorWriter requires a path"); - ICEBERG_PRECHECK( - static_cast(options.load_previous_deletes), - "DeletionVectorWriter requires a load_previous_deletes callback (return " - "nullptr when a data file has no existing deletes)"); + ICEBERG_PRECHECK(options.load_previous_deletes != nullptr, + "DeletionVectorWriter requires a load_previous_deletes callback"); return std::unique_ptr( new DeletionVectorWriter(std::make_unique(std::move(options)))); } Status DeletionVectorWriter::Delete(std::string_view referenced_data_file, int64_t pos, - std::shared_ptr spec, - PartitionValues partition) { - return impl_->Delete(referenced_data_file, pos, std::move(spec), std::move(partition)); + const std::shared_ptr& spec, + const PartitionValues& partition) { + return impl_->Delete(referenced_data_file, pos, spec, partition); } Status DeletionVectorWriter::Delete(std::string_view referenced_data_file, const PositionDeleteIndex& positions, - std::shared_ptr spec, - PartitionValues partition) { - return impl_->Delete(referenced_data_file, positions, std::move(spec), - std::move(partition)); + const std::shared_ptr& spec, + const PartitionValues& partition) { + return impl_->Delete(referenced_data_file, positions, spec, partition); } Status DeletionVectorWriter::Close() { return impl_->Close(); } -Result DeletionVectorWriter::Metadata() { return impl_->Metadata(); } +Result DeletionVectorWriter::Metadata() { return impl_->Metadata(); } } // namespace iceberg diff --git a/src/iceberg/data/deletion_vector_writer.h b/src/iceberg/data/deletion_vector_writer.h index 86856b7d8..bd5bb66a5 100644 --- a/src/iceberg/data/deletion_vector_writer.h +++ b/src/iceberg/data/deletion_vector_writer.h @@ -25,10 +25,9 @@ #include #include #include +#include #include #include -#include -#include #include "iceberg/data/writer.h" #include "iceberg/deletes/position_delete_index.h" @@ -41,51 +40,38 @@ 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 io; - /// File-level Puffin properties (e.g. "created-by"). - std::unordered_map properties; - /// Hook to load previously written deletes for a data file so they can be - /// merged into the new deletion vector. The returned index carries its source - /// delete files (via PositionDeleteIndex::delete_files()); file-scoped ones - /// are reported as rewritten. Return nullptr when there are none. Required: a - /// DV must replace any existing deletes for its data file. - std::function>(std::string_view)> + /// Loads existing deletes for a data file to merge, if any. + std::function>(std::string_view)> load_previous_deletes; }; -/// \brief Writes one or more deletion vectors into a single Puffin file. -/// -/// Each referenced data file gets its own `deletion-vector-v1` blob with its own -/// partition spec and partition. After Close(), Metadata() returns the produced -/// delete files plus the referenced and rewritten delete files. +/// \brief A deletion vector file writer. class ICEBERG_DATA_EXPORT DeletionVectorWriter { public: ~DeletionVectorWriter(); - DeletionVectorWriter(const DeletionVectorWriter&) = delete; - DeletionVectorWriter& operator=(const DeletionVectorWriter&) = delete; - /// \brief Create a new DeletionVectorWriter. static Result> Make( DeletionVectorWriterOptions options); /// \brief Mark a row position as deleted for the given data file. Status Delete(std::string_view referenced_data_file, int64_t pos, - std::shared_ptr spec, PartitionValues partition); + const std::shared_ptr& spec, + const PartitionValues& partition); /// \brief Mark all positions in the given index as deleted for a data file. Status Delete(std::string_view referenced_data_file, - const PositionDeleteIndex& positions, std::shared_ptr spec, - PartitionValues partition); + const PositionDeleteIndex& positions, + const std::shared_ptr& spec, + const PartitionValues& partition); /// \brief Write all accumulated deletion vectors to the Puffin file and close. Status Close(); /// \brief The result of writing; valid only after Close(). - Result Metadata(); + Result Metadata(); private: class Impl; diff --git a/src/iceberg/data/dv_util.cc b/src/iceberg/data/dv_util.cc new file mode 100644 index 000000000..b549197ca --- /dev/null +++ b/src/iceberg/data/dv_util.cc @@ -0,0 +1,68 @@ +/* + * 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 +#include +#include +#include +#include +#include + +#include "iceberg/data/dv_util_internal.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/file_io.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/util/content_file_util.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result DVUtil::ReadDV(const std::shared_ptr& delete_file, + const std::shared_ptr& io) { + ICEBERG_PRECHECK(delete_file != nullptr, "Delete file must not be null"); + ICEBERG_PRECHECK(io != nullptr, "DV read requires a FileIO"); + ICEBERG_PRECHECK(ContentFileUtil::IsDV(*delete_file), + "Cannot read, not a deletion vector: {}", delete_file->file_path); + ICEBERG_PRECHECK( + delete_file->content_offset.has_value() && + delete_file->content_size_in_bytes.has_value(), + "Deletion vector requires content_offset and content_size_in_bytes: {}", + delete_file->file_path); + + const int64_t offset = delete_file->content_offset.value(); + const int64_t length = delete_file->content_size_in_bytes.value(); + ICEBERG_PRECHECK(offset >= 0 && length >= 0, + "Invalid deletion vector offset/length: offset={}, length={}", offset, + length); + ICEBERG_PRECHECK(length <= std::numeric_limits::max(), + "Cannot read deletion vector larger than 2GB: {}", length); + + ICEBERG_ASSIGN_OR_RAISE(auto input_file, io->NewInputFile(delete_file->file_path)); + ICEBERG_ASSIGN_OR_RAISE(auto stream, input_file->Open()); + + std::vector bytes(static_cast(length)); + ICEBERG_RETURN_UNEXPECTED(stream->ReadFully(offset, bytes)); + ICEBERG_RETURN_UNEXPECTED(stream->Close()); + + std::span blob(reinterpret_cast(bytes.data()), + bytes.size()); + return PositionDeleteIndex::Deserialize(blob, delete_file); +} + +} // namespace iceberg diff --git a/src/iceberg/data/dv_util_internal.h b/src/iceberg/data/dv_util_internal.h new file mode 100644 index 000000000..4738b55bb --- /dev/null +++ b/src/iceberg/data/dv_util_internal.h @@ -0,0 +1,40 @@ +/* + * 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/dv_util_internal.h +/// Internal deletion vector helpers. + +#include + +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/iceberg_data_export.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" + +namespace iceberg { + +class ICEBERG_DATA_EXPORT DVUtil { + public: + static Result ReadDV(const std::shared_ptr& delete_file, + const std::shared_ptr& io); +}; + +} // namespace iceberg diff --git a/src/iceberg/data/equality_delete_writer.cc b/src/iceberg/data/equality_delete_writer.cc index 6604c03e4..89e6485b0 100644 --- a/src/iceberg/data/equality_delete_writer.cc +++ b/src/iceberg/data/equality_delete_writer.cc @@ -57,7 +57,7 @@ class EqualityDeleteWriter::Impl { return {}; } - Result Metadata() { + Result Metadata() { ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer"); ICEBERG_ASSIGN_OR_RAISE(auto metrics, writer_->metrics()); @@ -99,7 +99,7 @@ class EqualityDeleteWriter::Impl { options_.spec ? std::make_optional(options_.spec->spec_id()) : std::nullopt, }); - FileWriter::WriteResult result; + WriteResult result; result.data_files.push_back(std::move(data_file)); return result; } @@ -134,9 +134,7 @@ Result EqualityDeleteWriter::Length() const { return impl_->Length(); } Status EqualityDeleteWriter::Close() { return impl_->Close(); } -Result EqualityDeleteWriter::Metadata() { - return impl_->Metadata(); -} +Result EqualityDeleteWriter::Metadata() { return impl_->Metadata(); } std::span EqualityDeleteWriter::equality_field_ids() const { return impl_->equality_field_ids(); diff --git a/src/iceberg/data/position_delete_writer.cc b/src/iceberg/data/position_delete_writer.cc index 3238dc506..8c0e7a58a 100644 --- a/src/iceberg/data/position_delete_writer.cc +++ b/src/iceberg/data/position_delete_writer.cc @@ -92,7 +92,7 @@ class PositionDeleteWriter::Impl { return {}; } - Result Metadata() { + Result Metadata() { ICEBERG_CHECK(closed_, "Cannot get metadata before closing the writer"); ICEBERG_ASSIGN_OR_RAISE(auto metrics, writer_->metrics()); @@ -159,7 +159,7 @@ class PositionDeleteWriter::Impl { options_.spec ? std::make_optional(options_.spec->spec_id()) : std::nullopt, }); - FileWriter::WriteResult result; + WriteResult result; result.data_files.push_back(std::move(data_file)); return result; } @@ -233,8 +233,6 @@ Result PositionDeleteWriter::Length() const { return impl_->Length(); } Status PositionDeleteWriter::Close() { return impl_->Close(); } -Result PositionDeleteWriter::Metadata() { - return impl_->Metadata(); -} +Result PositionDeleteWriter::Metadata() { return impl_->Metadata(); } } // namespace iceberg diff --git a/src/iceberg/data/writer.h b/src/iceberg/data/writer.h index f47026c7e..0efcdc274 100644 --- a/src/iceberg/data/writer.h +++ b/src/iceberg/data/writer.h @@ -34,14 +34,12 @@ namespace iceberg { -/// \brief The result of writing delete files. -/// -/// Holds the delete files produced, the data files they reference, and any -/// previously written delete files that were merged and should be removed from -/// table state. -struct ICEBERG_DATA_EXPORT DeleteWriteResult { - /// Delete files produced. - std::vector> delete_files; +/// \brief File metadata for files produced by a writer. +struct ICEBERG_DATA_EXPORT WriteResult { + /// Usually a writer produces a single data or delete file. + /// Position delete writer may produce multiple file-scoped delete files. + /// In the future, multiple files can be produced if file rolling is supported. + std::vector> data_files; /// Data files referenced by the produced delete files. std::vector referenced_data_files; /// Previously written delete files that were merged and should be removed. @@ -63,14 +61,6 @@ class ICEBERG_DATA_EXPORT FileWriter { /// \brief Close the writer and finalize the file. virtual Status Close() = 0; - /// \brief File metadata for all files produced by this writer. - struct ICEBERG_DATA_EXPORT WriteResult { - /// Usually a writer produces a single data or delete file. - /// Position delete writer may produce multiple file-scoped delete files. - /// In the future, multiple files can be produced if file rolling is supported. - std::vector> data_files; - }; - /// \brief Get file metadata for all files produced by this writer. /// \note This method should be called after Close(). virtual Result Metadata() = 0; diff --git a/src/iceberg/deletes/position_delete_index.cc b/src/iceberg/deletes/position_delete_index.cc index 92f76f109..0d70cca4c 100644 --- a/src/iceberg/deletes/position_delete_index.cc +++ b/src/iceberg/deletes/position_delete_index.cc @@ -50,17 +50,54 @@ uint32_t ComputeCrc32(std::span bytes) { return static_cast(crc); } -template -void WriteBigEndian(T value, uint8_t* buf) { - T be = ToBigEndian(value); - std::memcpy(buf, &be, sizeof(be)); +Result ReadBitmapDataLength(std::span blob, + const DataFile& delete_file) { + ICEBERG_PRECHECK(delete_file.content_size_in_bytes.has_value(), + "Deletion vector requires content_size_in_bytes: {}", + delete_file.file_path); + ICEBERG_PRECHECK( + std::cmp_equal(blob.size(), *delete_file.content_size_in_bytes), + "Deletion vector blob size {} does not match content_size_in_bytes {}: {}", + blob.size(), *delete_file.content_size_in_bytes, delete_file.file_path); + ICEBERG_PRECHECK(blob.size() >= kLengthPrefixBytes + kMagicBytes + kCrcBytes, + "Deletion vector blob too small: {} bytes", blob.size()); + + const auto length = ReadBigEndian(blob.data()); + const size_t expected_size = + kLengthPrefixBytes + static_cast(length) + kCrcBytes; + ICEBERG_PRECHECK(length >= kMagicBytes, "Invalid deletion vector length prefix: {}", + length); + ICEBERG_PRECHECK(blob.size() == expected_size, + "Deletion vector blob size mismatch: {} bytes, expected {}", + blob.size(), expected_size); + return length; +} + +Result DeserializeBitmap(std::span blob, + int32_t length, + const DataFile& delete_file) { + const uint8_t* bitmap_data = blob.data() + kLengthPrefixBytes; + ICEBERG_PRECHECK(std::memcmp(bitmap_data, kMagic.data(), kMagic.size()) == 0, + "Invalid deletion vector magic"); + + std::string_view vector_bytes(reinterpret_cast(bitmap_data + kMagicBytes), + static_cast(length) - kMagicBytes); + ICEBERG_ASSIGN_OR_RAISE(auto bitmap, RoaringPositionBitmap::Deserialize(vector_bytes)); + ICEBERG_PRECHECK(std::cmp_equal(bitmap.Cardinality(), delete_file.record_count), + "Deletion vector cardinality {} does not match record count {}: {}", + bitmap.Cardinality(), delete_file.record_count, delete_file.file_path); + return bitmap; } -template -T ReadBigEndian(const uint8_t* buf) { - T value; - std::memcpy(&value, buf, sizeof(value)); - return FromBigEndian(value); +Status ValidateChecksum(std::span blob, int32_t length) { + const uint8_t* bitmap_data = blob.data() + kLengthPrefixBytes; + const auto stored_crc = ReadBigEndian(bitmap_data + length); + const auto actual_crc = + ComputeCrc32(std::span(bitmap_data, static_cast(length))); + ICEBERG_PRECHECK(stored_crc == actual_crc, + "Deletion vector CRC mismatch: stored {:#010x}, computed {:#010x}", + stored_crc, actual_crc); + return {}; } } // namespace @@ -68,6 +105,16 @@ T ReadBigEndian(const uint8_t* buf) { PositionDeleteIndex::PositionDeleteIndex(RoaringPositionBitmap bitmap) : bitmap_(std::move(bitmap)) {} +PositionDeleteIndex::PositionDeleteIndex(std::shared_ptr delete_file) { + if (delete_file != nullptr) { + delete_files_.push_back(std::move(delete_file)); + } +} + +PositionDeleteIndex::PositionDeleteIndex( + std::vector> delete_files) + : delete_files_(std::move(delete_files)) {} + void PositionDeleteIndex::Delete(int64_t pos) { bitmap_.Add(pos); } void PositionDeleteIndex::Delete(int64_t pos_start, int64_t pos_end) { @@ -88,35 +135,23 @@ void PositionDeleteIndex::Merge(const PositionDeleteIndex& other) { other.delete_files_.end()); } -void PositionDeleteIndex::AddDeleteFile(std::shared_ptr delete_file) { - delete_files_.push_back(std::move(delete_file)); -} - Result> PositionDeleteIndex::Serialize() { bitmap_.Optimize(); // run-length encode before serializing - ICEBERG_ASSIGN_OR_RAISE(auto vector, bitmap_.Serialize()); - - // The length prefix and CRC both cover the magic sequence plus the vector. - const size_t magic_and_vector_size = static_cast(kMagicBytes) + vector.size(); - ICEBERG_PRECHECK( - magic_and_vector_size <= static_cast(std::numeric_limits::max()), - "Deletion vector is too large to serialize: {} bytes", magic_and_vector_size); - - std::vector blob(static_cast(kLengthPrefixBytes) + - magic_and_vector_size + static_cast(kCrcBytes)); - uint8_t* buf = blob.data(); - - WriteBigEndian(static_cast(magic_and_vector_size), buf); - buf += kLengthPrefixBytes; - - uint8_t* checksum_begin = buf; - std::memcpy(buf, kMagic.data(), kMagicBytes); - buf += kMagicBytes; - std::memcpy(buf, vector.data(), vector.size()); - buf += vector.size(); - - WriteBigEndian( - ComputeCrc32(std::span(checksum_begin, magic_and_vector_size)), buf); + std::vector blob(kLengthPrefixBytes); + blob.insert(blob.end(), kMagic.begin(), kMagic.end()); + ICEBERG_ASSIGN_OR_RAISE(const auto vector_size, bitmap_.SerializeTo(blob)); + + const size_t magic_and_vector_size = kMagicBytes + vector_size; + ICEBERG_PRECHECK(magic_and_vector_size <= std::numeric_limits::max(), + "Deletion vector is too large to serialize: {} bytes", + magic_and_vector_size); + + WriteBigEndian(static_cast(magic_and_vector_size), blob.data()); + const auto crc_offset = blob.size(); + blob.resize(crc_offset + kCrcBytes); + WriteBigEndian(ComputeCrc32(std::span(blob.data() + kLengthPrefixBytes, + magic_and_vector_size)), + blob.data() + crc_offset); return blob; } @@ -124,65 +159,10 @@ Result PositionDeleteIndex::Deserialize( std::span blob, std::shared_ptr delete_file) { ICEBERG_PRECHECK(delete_file != nullptr, "Deletion vector requires a source delete file"); - // DV metadata requires content_size_in_bytes; the blob bytes must match it. - ICEBERG_PRECHECK(delete_file->content_size_in_bytes.has_value(), - "Deletion vector requires content_size_in_bytes: {}", - delete_file->file_path); - ICEBERG_PRECHECK( - std::cmp_equal(blob.size(), delete_file->content_size_in_bytes.value()), - "Deletion vector blob size {} does not match content_size_in_bytes {}: {}", - blob.size(), delete_file->content_size_in_bytes.value(), delete_file->file_path); - - constexpr size_t kMinSize = static_cast(kLengthPrefixBytes) + - static_cast(kMagicBytes) + - static_cast(kCrcBytes); - ICEBERG_PRECHECK(blob.size() >= kMinSize, - "Deletion vector blob too small: {} bytes, need at least {}", - blob.size(), kMinSize); - - const uint8_t* buf = blob.data(); - - const auto length = ReadBigEndian(buf); - buf += kLengthPrefixBytes; - - ICEBERG_PRECHECK(length >= kMagicBytes, "Invalid deletion vector length prefix: {}", - length); - - const size_t expected_total = static_cast(kLengthPrefixBytes) + - static_cast(length) + - static_cast(kCrcBytes); - ICEBERG_PRECHECK(blob.size() == expected_total, - "Deletion vector blob size mismatch: {} bytes, expected {}", - blob.size(), expected_total); - - // Magic and vector are checksummed together by the trailing CRC. - const uint8_t* checksum_begin = buf; - for (size_t i = 0; i < kMagic.size(); ++i) { - ICEBERG_PRECHECK(buf[i] == kMagic[i], - "Invalid deletion vector magic byte at offset {}: got {:#04x}", i, - buf[i]); - } - buf += kMagicBytes; - - const auto stored_crc = - ReadBigEndian(checksum_begin + static_cast(length)); - const uint32_t actual_crc = - ComputeCrc32(std::span(checksum_begin, static_cast(length))); - ICEBERG_PRECHECK(stored_crc == actual_crc, - "Deletion vector CRC mismatch: stored {:#010x}, computed {:#010x}", - stored_crc, actual_crc); - - const auto vector_size = static_cast(length) - kMagicBytes; - std::string_view vector_bytes(reinterpret_cast(buf), vector_size); - ICEBERG_ASSIGN_OR_RAISE(auto bitmap, RoaringPositionBitmap::Deserialize(vector_bytes)); + ICEBERG_ASSIGN_OR_RAISE(const auto length, ReadBitmapDataLength(blob, *delete_file)); + ICEBERG_ASSIGN_OR_RAISE(auto bitmap, DeserializeBitmap(blob, length, *delete_file)); + ICEBERG_RETURN_UNEXPECTED(ValidateChecksum(blob, length)); PositionDeleteIndex index(std::move(bitmap)); - - // The bitmap cardinality must match the record count recorded in metadata. - ICEBERG_PRECHECK(std::cmp_equal(index.Cardinality(), delete_file->record_count), - "Deletion vector cardinality {} does not match record count {}: {}", - index.Cardinality(), delete_file->record_count, - delete_file->file_path); - index.delete_files_.push_back(std::move(delete_file)); return index; } diff --git a/src/iceberg/deletes/position_delete_index.h b/src/iceberg/deletes/position_delete_index.h index 02c792a9d..cbf2db478 100644 --- a/src/iceberg/deletes/position_delete_index.h +++ b/src/iceberg/deletes/position_delete_index.h @@ -30,11 +30,10 @@ #include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/iceberg_data_export.h" #include "iceberg/result.h" +#include "iceberg/type_fwd.h" namespace iceberg { -struct DataFile; - /// \brief Tracks deleted row positions using a bitmap. /// /// This class provides a domain-specific API for position deletes @@ -43,6 +42,8 @@ struct DataFile; class ICEBERG_DATA_EXPORT PositionDeleteIndex { public: PositionDeleteIndex() = default; + explicit PositionDeleteIndex(std::shared_ptr delete_file); + explicit PositionDeleteIndex(std::vector> delete_files); ~PositionDeleteIndex() = default; /// \brief Mark a position as deleted. @@ -71,19 +72,13 @@ class ICEBERG_DATA_EXPORT PositionDeleteIndex { /// \brief The delete files whose positions were merged into this index. /// - /// Populated by Deserialize and AddDeleteFile, and preserved across Merge. + /// Populated by constructors and Deserialize, and preserved across Merge. /// Callers use these to report the delete files that were rewritten when /// replacing them with a new deletion vector. const std::vector>& delete_files() const { return delete_files_; } - /// \brief Record a delete file whose positions were merged into this index. - /// - /// Used by the position-delete load path, where positions come from a Parquet - /// delete file rather than a DV blob. The file is exposed via delete_files(). - void AddDeleteFile(std::shared_ptr delete_file); - /// \brief Serialize the index into a `deletion-vector-v1` blob. /// /// The positions are run-length encoded, then framed per the Puffin spec: diff --git a/src/iceberg/deletes/roaring_position_bitmap.cc b/src/iceberg/deletes/roaring_position_bitmap.cc index e31efeec1..93624d2bf 100644 --- a/src/iceberg/deletes/roaring_position_bitmap.cc +++ b/src/iceberg/deletes/roaring_position_bitmap.cc @@ -57,6 +57,16 @@ Status ValidatePosition(int64_t pos) { return {}; } +void WriteBitmaps(const std::vector& bitmaps, uint8_t* buf) { + WriteLittleEndian(static_cast(bitmaps.size()), buf); + buf += kBitmapCountSizeBytes; + for (int32_t key = 0; std::cmp_less(key, bitmaps.size()); ++key) { + WriteLittleEndian(key, buf); + buf += kBitmapKeySizeBytes; + buf += bitmaps[key].write(reinterpret_cast(buf), /*portable=*/true); + } +} + } // namespace struct RoaringPositionBitmap::Impl { @@ -186,23 +196,18 @@ size_t RoaringPositionBitmap::SerializedSizeInBytes() const { Result RoaringPositionBitmap::Serialize() const { size_t size = SerializedSizeInBytes(); std::string result(size, '\0'); - char* buf = result.data(); - - // Write bitmap count (array length including empties) - WriteLittleEndian(static_cast(impl_->bitmaps.size()), buf); - buf += kBitmapCountSizeBytes; - - // Write each bitmap with its key - for (int32_t key = 0; std::cmp_less(key, impl_->bitmaps.size()); ++key) { - WriteLittleEndian(key, buf); - buf += kBitmapKeySizeBytes; - size_t written = impl_->bitmaps[key].write(buf, /*portable=*/true); - buf += written; - } - + WriteBitmaps(impl_->bitmaps, reinterpret_cast(result.data())); return result; } +Result RoaringPositionBitmap::SerializeTo(std::vector& output) const { + const size_t offset = output.size(); + const size_t size = SerializedSizeInBytes(); + output.resize(offset + size); + WriteBitmaps(impl_->bitmaps, output.data() + offset); + return size; +} + Result RoaringPositionBitmap::Deserialize(std::string_view bytes) { const char* buf = bytes.data(); size_t remaining = bytes.size(); diff --git a/src/iceberg/deletes/roaring_position_bitmap.h b/src/iceberg/deletes/roaring_position_bitmap.h index bfb7d7c9e..50807dc6a 100644 --- a/src/iceberg/deletes/roaring_position_bitmap.h +++ b/src/iceberg/deletes/roaring_position_bitmap.h @@ -28,6 +28,7 @@ #include #include #include +#include #include "iceberg/iceberg_data_export.h" #include "iceberg/result.h" @@ -118,6 +119,8 @@ class ICEBERG_DATA_EXPORT RoaringPositionBitmap { // `PositionDeleteIndex::BulkAddForKey`; per-key grouping is the caller's // job, keeping this a thin wrapper around CRoaring's `addMany`. void AddManyForKey(int32_t key, std::span positions); + // Appends the portable serialized bitmap and returns bytes appended. + Result SerializeTo(std::vector& output) const; friend class PositionDeleteIndex; }; diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index cc2c683e0..e3bef01af 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -18,6 +18,7 @@ conf_data = configuration_data() version = meson.project_version() version_suffix = '-SNAPSHOT' +version_string = version + version_suffix components = version.split('.') assert( components.length() >= 3, @@ -29,7 +30,30 @@ ver_patch = components[2] conf_data.set('PROJECT_VERSION_MAJOR', ver_major) conf_data.set('PROJECT_VERSION_MINOR', ver_minor) conf_data.set('PROJECT_VERSION_PATCH', ver_patch) -conf_data.set('ICEBERG_VERSION_STRING', version + version_suffix) +conf_data.set('ICEBERG_VERSION_STRING', version_string) +git = find_program('git', required: false) +git_commit_id = 'unknown' +if git.found() + git_result = run_command( + git, + '-C', + meson.project_source_root(), + 'rev-parse', + 'HEAD', + check: false, + ) + if git_result.returncode() == 0 + git_commit_id_candidate = git_result.stdout().strip() + if git_commit_id_candidate != '' + git_commit_id = git_commit_id_candidate + endif + endif +endif +conf_data.set('ICEBERG_GIT_COMMIT_ID', git_commit_id) +conf_data.set( + 'ICEBERG_FULL_VERSION_STRING', + 'Apache Iceberg-CPP ' + version_string + ' (commit ' + git_commit_id + ')', +) conf_data.set('PROJECT_NAME', meson.project_name()) configure_file( input: 'version.h.in', @@ -175,6 +199,7 @@ iceberg_data_sources = files( 'data/delete_filter.cc', 'data/delete_loader.cc', 'data/deletion_vector_writer.cc', + 'data/dv_util.cc', 'data/equality_delete_writer.cc', 'data/file_scan_task_reader.cc', 'data/position_delete_writer.cc', diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index a60e3ac82..98129a6d2 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -162,9 +162,6 @@ add_iceberg_test(puffin_test puffin_json_test.cc puffin_reader_writer_test.cc) -add_iceberg_test(deletion_vector_writer_test USE_DATA SOURCES - deletion_vector_writer_test.cc) - if(ICEBERG_BUILD_BUNDLE) add_iceberg_test(avro_test USE_BUNDLE @@ -257,6 +254,7 @@ if(ICEBERG_BUILD_BUNDLE) data_writer_test.cc delete_filter_test.cc delete_loader_test.cc + deletion_vector_writer_test.cc file_scan_task_reader_test.cc literal_util_test.cc) diff --git a/src/iceberg/test/delete_filter_test.cc b/src/iceberg/test/delete_filter_test.cc index 8ae8ff7e8..d4ef94960 100644 --- a/src/iceberg/test/delete_filter_test.cc +++ b/src/iceberg/test/delete_filter_test.cc @@ -38,6 +38,7 @@ #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" #include "iceberg/file_format.h" +#include "iceberg/file_io.h" #include "iceberg/file_reader.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/metadata_columns.h" @@ -257,7 +258,7 @@ class DeleteFilterTest : public ::testing::Test { .path = path, .io = file_io_, .load_previous_deletes = [](std::string_view) - -> Result> { return nullptr; }, + -> Result> { return std::nullopt; }, }; ICEBERG_ASSIGN_OR_RAISE(auto writer, DeletionVectorWriter::Make(std::move(options))); for (int64_t pos : positions) { @@ -266,7 +267,7 @@ class DeleteFilterTest : public ::testing::Test { } ICEBERG_RETURN_UNEXPECTED(writer->Close()); ICEBERG_ASSIGN_OR_RAISE(auto metadata, writer->Metadata()); - return metadata.delete_files[0]; + return metadata.data_files[0]; } std::shared_ptr file_io_; diff --git a/src/iceberg/test/delete_loader_test.cc b/src/iceberg/test/delete_loader_test.cc index 225ef19cd..db166875d 100644 --- a/src/iceberg/test/delete_loader_test.cc +++ b/src/iceberg/test/delete_loader_test.cc @@ -19,6 +19,8 @@ #include "iceberg/data/delete_loader.h" +#include + #include #include #include @@ -117,19 +119,20 @@ class DeleteLoaderTest : public ::testing::Test { const std::string& referenced_data_file, const std::vector& positions) { auto writer = - DeletionVectorWriter::Make( - DeletionVectorWriterOptions{ - .path = path, - .io = file_io_, - .load_previous_deletes = [](std::string_view) - -> Result> { return nullptr; }}) + DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = path, + .io = file_io_, + .load_previous_deletes = [](std::string_view) + -> Result> { + return std::nullopt; + }}) .value(); for (int64_t pos : positions) { ICEBERG_THROW_NOT_OK( writer->Delete(referenced_data_file, pos, partition_spec_, PartitionValues{})); } ICEBERG_THROW_NOT_OK(writer->Close()); - return writer->Metadata().value().delete_files[0]; + return writer->Metadata().value().data_files[0]; } std::shared_ptr file_io_; @@ -191,8 +194,7 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesFiltersByDataFilePath) { {"data_b.parquet", 10}, {"data_b.parquet", 20}}); - // Mixed paths -> writer must NOT set the hint, forcing the loader's - // per-row filter path. Locks the routing in case the writer behavior changes. + // Mixed paths keep the loader on the per-row filter path. ASSERT_FALSE(delete_file->referenced_data_file.has_value()); std::vector> files = {delete_file}; @@ -234,9 +236,7 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceDeleteFile) { EXPECT_EQ(result.value().delete_files()[0], file_scoped); } -// The filter path (mixed paths, no hint) also records the source file, but only -// when the target data file actually has matching rows. -TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceOnlyWhenMatched) { +TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceForFilterPath) { auto mixed = WritePositionDeletes("pos_src_mixed.parquet", {{"data_a.parquet", 0}, {"data_b.parquet", 10}}); ASSERT_FALSE(mixed->referenced_data_file.has_value()); @@ -248,10 +248,11 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesTracksSourceOnlyWhenMatched) { ASSERT_EQ(matched.value().delete_files().size(), 1u); EXPECT_EQ(matched.value().delete_files()[0], mixed); - // No matching rows -> the file is not recorded. auto unmatched = loader_->LoadPositionDeletes(files, "data_c.parquet"); ASSERT_THAT(unmatched, IsOk()); - EXPECT_TRUE(unmatched.value().delete_files().empty()); + ASSERT_TRUE(unmatched.value().IsEmpty()); + ASSERT_EQ(unmatched.value().delete_files().size(), 1u); + EXPECT_EQ(unmatched.value().delete_files()[0], mixed); } TEST_F(DeleteLoaderTest, LoadPositionDeletesRejectsNullFile) { @@ -275,10 +276,7 @@ TEST_F(DeleteLoaderTest, LoadPositionDeletesSkipsMismatchedReferencedDataFile) { } TEST_F(DeleteLoaderTest, LoadPositionDeletesFastPathHonorsReferencedDataFile) { - // Single-file writes -> writer sets referenced_data_file -> loader takes - // the zero-copy fast path. Sized above the consumer's 64-element sniff - // threshold so the dispatcher's real coalesce/bulk logic runs end-to-end, - // not just the small-input shortcut covered by LoadPositionDeletesSingleFile. + // Large enough to exercise the bulk path. constexpr int64_t kRowCount = 128; std::vector> deletes; deletes.reserve(kRowCount); @@ -366,13 +364,13 @@ TEST_F(DeleteLoaderTest, DVWriterReportsFileScopedPositionDeleteAsRewritten) { .path = "dv-replace.puffin", .io = file_io_, .load_previous_deletes = - [&](std::string_view path) -> Result> { + [&](std::string_view path) -> Result> { std::vector> files = {previous}; ICEBERG_ASSIGN_OR_RAISE(auto index, loader->LoadPositionDeletes(files, path)); if (index.IsEmpty()) { - return nullptr; + return std::nullopt; } - return std::make_shared(std::move(index)); + return std::optional(std::move(index)); }, })); @@ -381,9 +379,9 @@ TEST_F(DeleteLoaderTest, DVWriterReportsFileScopedPositionDeleteAsRewritten) { ICEBERG_THROW_NOT_OK(writer->Close()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - ASSERT_EQ(result.delete_files.size(), 1u); + ASSERT_EQ(result.data_files.size(), 1u); // New position plus the two previous positions. - EXPECT_EQ(result.delete_files[0]->record_count, 3); + EXPECT_EQ(result.data_files[0]->record_count, 3); // The previous Parquet position delete is file-scoped and must be reported. ASSERT_EQ(result.rewritten_delete_files.size(), 1u); EXPECT_EQ(result.rewritten_delete_files[0], previous); diff --git a/src/iceberg/test/deletion_vector_writer_test.cc b/src/iceberg/test/deletion_vector_writer_test.cc index a4a0e1aea..7e3cf050f 100644 --- a/src/iceberg/test/deletion_vector_writer_test.cc +++ b/src/iceberg/test/deletion_vector_writer_test.cc @@ -19,8 +19,11 @@ #include "iceberg/data/deletion_vector_writer.h" +#include #include +#include #include +#include #include #include @@ -30,10 +33,14 @@ #include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/file_format.h" #include "iceberg/manifest/manifest_entry.h" +#include "iceberg/metadata_columns.h" #include "iceberg/partition_spec.h" +#include "iceberg/puffin/file_metadata.h" +#include "iceberg/puffin/puffin_reader.h" #include "iceberg/row/partition_values.h" #include "iceberg/test/matchers.h" #include "iceberg/test/mock_io.h" +#include "iceberg/version.h" namespace iceberg { @@ -53,11 +60,33 @@ std::shared_ptr UnpartitionedSpec() { return PartitionSpec::Unpartitioned(); } -// A load_previous_deletes callback for data files that have no existing deletes. -Result> NoPreviousDeletes(std::string_view) { +const puffin::BlobMetadata* FindBlobByReferencedFile( + const std::vector& blobs, const std::string& ref) { + for (const auto& blob : blobs) { + auto it = blob.properties.find("referenced-data-file"); + if (it != blob.properties.end() && it->second == ref) { + return &blob; + } + } return nullptr; } +// A load_previous_deletes callback for data files that have no existing deletes. +Result> NoPreviousDeletes(std::string_view) { + return std::nullopt; +} + +DeletionVectorWriterOptions MakeDVWriterOptions( + std::shared_ptr io, std::string path, + std::function>(std::string_view)> + load_previous_deletes = NoPreviousDeletes) { + return DeletionVectorWriterOptions{ + .path = std::move(path), + .io = std::move(io), + .load_previous_deletes = std::move(load_previous_deletes), + }; +} + } // namespace TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { @@ -66,13 +95,8 @@ TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { std::vector> delete_files; { - ICEBERG_UNWRAP_OR_FAIL(auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://deletes.puffin", - .io = io, - .properties = {{"created-by", "iceberg-cpp-test"}}, - .load_previous_deletes = NoPreviousDeletes, - })); + ICEBERG_UNWRAP_OR_FAIL(auto writer, DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://deletes.puffin"))); ASSERT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), IsOk()); ASSERT_THAT(writer->Delete("data-a.parquet", 5, spec, PartitionValues{}), IsOk()); @@ -82,7 +106,7 @@ TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { ASSERT_THAT(writer->Close(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - delete_files = result.delete_files; + delete_files = result.data_files; // Each referenced data file is reported once. EXPECT_EQ(result.referenced_data_files.size(), 2u); // No previous deletes were loaded, so nothing was rewritten. @@ -112,9 +136,34 @@ TEST(DeletionVectorWriterTest, WriteThenLoadEndToEnd) { EXPECT_EQ(dv_a->file_path, dv_b->file_path); EXPECT_NE(dv_a->content_offset.value(), dv_b->content_offset.value()); - // Read back through DeleteLoader. Each data file's DV is applied on its own - // (a single DV replaces all other deletes for that file), so the loader is - // called with just the DV that references the requested data file. + ICEBERG_UNWRAP_OR_FAIL(auto input_file, io->NewInputFile("memory://deletes.puffin")); + ICEBERG_UNWRAP_OR_FAIL(auto puffin_reader, + puffin::PuffinReader::Make(std::move(input_file))); + ICEBERG_UNWRAP_OR_FAIL(auto puffin_metadata, puffin_reader->ReadFileMetadata()); + ASSERT_EQ(puffin_metadata.blobs.size(), 2u); + EXPECT_EQ(puffin_metadata.properties.at("created-by"), ICEBERG_FULL_VERSION_STRING); + + auto assert_blob = [](const puffin::BlobMetadata& blob, const DataFile& file, + const std::string& ref, const std::string& cardinality) { + EXPECT_EQ(blob.type, puffin::StandardBlobTypes::kDeletionVectorV1); + EXPECT_EQ(blob.input_fields, + std::vector{MetadataColumns::kFilePositionColumnId}); + EXPECT_EQ(blob.snapshot_id, -1); + EXPECT_EQ(blob.sequence_number, -1); + EXPECT_TRUE(blob.compression_codec.empty()); + EXPECT_EQ(blob.offset, file.content_offset.value()); + EXPECT_EQ(blob.length, file.content_size_in_bytes.value()); + EXPECT_EQ(blob.properties.at("referenced-data-file"), ref); + EXPECT_EQ(blob.properties.at("cardinality"), cardinality); + }; + ASSERT_NE(FindBlobByReferencedFile(puffin_metadata.blobs, "data-a.parquet"), nullptr); + ASSERT_NE(FindBlobByReferencedFile(puffin_metadata.blobs, "data-b.parquet"), nullptr); + assert_blob(*FindBlobByReferencedFile(puffin_metadata.blobs, "data-a.parquet"), *dv_a, + "data-a.parquet", "3"); + assert_blob(*FindBlobByReferencedFile(puffin_metadata.blobs, "data-b.parquet"), *dv_b, + "data-b.parquet", "2"); + + // Load each data file's DV separately. DeleteLoader loader(io); { auto result = loader.LoadPositionDeletes({&dv_a, 1}, "data-a.parquet"); @@ -147,20 +196,17 @@ TEST(DeletionVectorWriterTest, DeleteFromIndex) { positions.Delete(0); positions.Delete(3, 6); // [3, 6) -> 3, 4, 5 - ICEBERG_UNWRAP_OR_FAIL(auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://from-index.puffin", - .io = io, - .load_previous_deletes = NoPreviousDeletes})); + ICEBERG_UNWRAP_OR_FAIL(auto writer, DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://from-index.puffin"))); ASSERT_THAT(writer->Delete("data.parquet", positions, spec, PartitionValues{}), IsOk()); ASSERT_THAT(writer->Close(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - ASSERT_EQ(result.delete_files.size(), 1u); - EXPECT_EQ(result.delete_files[0]->record_count, 4); + ASSERT_EQ(result.data_files.size(), 1u); + EXPECT_EQ(result.data_files[0]->record_count, 4); DeleteLoader loader(io); - auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + auto loaded = loader.LoadPositionDeletes(result.data_files, "data.parquet"); ASSERT_THAT(loaded, IsOk()); EXPECT_EQ(loaded.value().Cardinality(), 4); EXPECT_TRUE(loaded.value().IsDeleted(0)); @@ -192,33 +238,30 @@ TEST(DeletionVectorWriterTest, LoadPreviousDeletesMergesAndReportsRewritten) { ICEBERG_UNWRAP_OR_FAIL( auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://merged.puffin", - .io = io, - .load_previous_deletes = - [&](std::string_view path) -> Result> { + DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://merged.puffin", + [&](std::string_view path) -> Result> { if (path != "data.parquet") { - return nullptr; + return std::nullopt; } ICEBERG_ASSIGN_OR_RAISE( auto index, PositionDeleteIndex::Deserialize(previous_blob, previous_dv)); - return std::make_shared(std::move(index)); - }, - })); + return std::optional(std::move(index)); + }))); ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); ASSERT_THAT(writer->Close(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - ASSERT_EQ(result.delete_files.size(), 1u); + ASSERT_EQ(result.data_files.size(), 1u); // New position plus the two previous positions. - EXPECT_EQ(result.delete_files[0]->record_count, 3); + EXPECT_EQ(result.data_files[0]->record_count, 3); // The previous DV is file-scoped, so it is reported for removal. ASSERT_EQ(result.rewritten_delete_files.size(), 1u); EXPECT_EQ(result.rewritten_delete_files[0]->file_path, "memory://old.puffin"); DeleteLoader loader(io); - auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + auto loaded = loader.LoadPositionDeletes(result.data_files, "data.parquet"); ASSERT_THAT(loaded, IsOk()); EXPECT_EQ(loaded.value().Cardinality(), 3); EXPECT_TRUE(loaded.value().IsDeleted(0)); @@ -226,49 +269,32 @@ TEST(DeletionVectorWriterTest, LoadPreviousDeletesMergesAndReportsRewritten) { EXPECT_TRUE(loaded.value().IsDeleted(200)); } -// A previous delete that is not file-scoped (e.g. a partition-scoped position -// delete) is merged into the new vector but is NOT reported as rewritten. -TEST(DeletionVectorWriterTest, PartitionScopedPreviousDeleteMergesButNotRewritten) { +TEST(DeletionVectorWriterTest, PreviousDeletesWithoutSourceFilesAreNotRewritten) { auto io = std::make_shared(); auto spec = UnpartitionedSpec(); - // A partition-scoped Parquet position delete: no referenced_data_file, so it - // is not file-scoped and must not be reported as rewritten. - auto previous_position_delete = std::make_shared(DataFile{ - .content = DataFile::Content::kPositionDeletes, - .file_path = "memory://partition-deletes.parquet", - .file_format = FileFormatType::kParquet, - .record_count = 1, - }); - ICEBERG_UNWRAP_OR_FAIL( auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://merged-partition.puffin", - .io = io, - .load_previous_deletes = - [&](std::string_view path) -> Result> { - // Model what DeleteLoader produces from a Parquet position delete: - // positions plus the source delete file, not a DV blob. - auto index = std::make_shared(); - index->Delete(50); - index->AddDeleteFile(previous_position_delete); - return index; - }, - })); + DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://merged-partition.puffin", + [&](std::string_view path) -> Result> { + PositionDeleteIndex index; + index.Delete(50); + return std::optional(std::move(index)); + }))); ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); ASSERT_THAT(writer->Close(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - ASSERT_EQ(result.delete_files.size(), 1u); + ASSERT_EQ(result.data_files.size(), 1u); // The previous position was merged in. - EXPECT_EQ(result.delete_files[0]->record_count, 2); + EXPECT_EQ(result.data_files[0]->record_count, 2); // The previous delete is partition-scoped, so it is not rewritten. EXPECT_TRUE(result.rewritten_delete_files.empty()); DeleteLoader loader(io); - auto loaded = loader.LoadPositionDeletes(result.delete_files, "data.parquet"); + auto loaded = loader.LoadPositionDeletes(result.data_files, "data.parquet"); ASSERT_THAT(loaded, IsOk()); EXPECT_EQ(loaded.value().Cardinality(), 2); EXPECT_TRUE(loaded.value().IsDeleted(0)); @@ -284,32 +310,30 @@ TEST(DeletionVectorWriterTest, EmptyWriterProducesNoDataFiles) { .load_previous_deletes = NoPreviousDeletes})); ASSERT_THAT(writer->Close(), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto result, writer->Metadata()); - EXPECT_TRUE(result.delete_files.empty()); - - // No blobs were written, so no (orphan) Puffin file should have been created. + EXPECT_TRUE(result.data_files.empty()); EXPECT_THAT(io->NewInputFile("memory://empty.puffin"), IsError(ErrorKind::kNotFound)); } TEST(DeletionVectorWriterTest, DeleteRejectsEmptyReferencedFile) { auto io = std::make_shared(); auto spec = UnpartitionedSpec(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://invalid.puffin", - .io = io, - .load_previous_deletes = NoPreviousDeletes})); + ICEBERG_UNWRAP_OR_FAIL(auto writer, DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://invalid.puffin"))); EXPECT_THAT(writer->Delete("", 0, spec, PartitionValues{}), IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(writer->Delete("data.parquet", 0, nullptr, PartitionValues{}), + IsError(ErrorKind::kInvalidArgument)); + PositionDeleteIndex positions; + positions.Delete(0); + EXPECT_THAT(writer->Delete("data.parquet", positions, nullptr, PartitionValues{}), + IsError(ErrorKind::kInvalidArgument)); } TEST(DeletionVectorWriterTest, DeleteRejectsOutOfRangePosition) { auto io = std::make_shared(); auto spec = UnpartitionedSpec(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://invalid.puffin", - .io = io, - .load_previous_deletes = NoPreviousDeletes})); + ICEBERG_UNWRAP_OR_FAIL(auto writer, DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://invalid.puffin"))); // Negative and out-of-range positions are rejected rather than silently // dropped by the underlying bitmap. EXPECT_THAT(writer->Delete("data.parquet", -1, spec, PartitionValues{}), @@ -325,12 +349,11 @@ TEST(DeletionVectorWriterTest, ClosePropagatesLoadPreviousDeletesError) { auto spec = UnpartitionedSpec(); ICEBERG_UNWRAP_OR_FAIL( auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://err.puffin", - .io = io, - .load_previous_deletes = [](std::string_view) - -> Result> { return IOError("boom"); }, - })); + DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://err.puffin", + [](std::string_view) -> Result> { + return IOError("boom"); + }))); ASSERT_THAT(writer->Delete("data.parquet", 0, spec, PartitionValues{}), IsOk()); EXPECT_THAT(writer->Close(), IsError(ErrorKind::kIOError)); @@ -340,34 +363,27 @@ TEST(DeletionVectorWriterTest, ClosePropagatesLoadPreviousDeletesError) { EXPECT_THAT(io->NewInputFile("memory://err.puffin"), IsError(ErrorKind::kNotFound)); } -TEST(DeletionVectorWriterTest, MakeRejectsNullIo) { +TEST(DeletionVectorWriterTest, MakeRejectsMissingOutputPath) { EXPECT_THAT(DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "x.puffin", .load_previous_deletes = NoPreviousDeletes}), - IsError(ErrorKind::kInvalidArgument)); -} - -TEST(DeletionVectorWriterTest, MakeRejectsEmptyPath) { - auto io = std::make_shared(); - EXPECT_THAT(DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .io = io, .load_previous_deletes = NoPreviousDeletes}), + .io = std::make_shared(), + .load_previous_deletes = NoPreviousDeletes}), IsError(ErrorKind::kInvalidArgument)); } TEST(DeletionVectorWriterTest, MakeRejectsMissingLoadPreviousDeletes) { auto io = std::make_shared(); - EXPECT_THAT(DeletionVectorWriter::Make( - DeletionVectorWriterOptions{.path = "x.puffin", .io = io}), + EXPECT_THAT(DeletionVectorWriter::Make(DeletionVectorWriterOptions{ + .path = "x.puffin", + .io = io, + }), IsError(ErrorKind::kInvalidArgument)); } TEST(DeletionVectorWriterTest, DeleteAfterCloseFails) { auto io = std::make_shared(); auto spec = UnpartitionedSpec(); - ICEBERG_UNWRAP_OR_FAIL(auto writer, - DeletionVectorWriter::Make(DeletionVectorWriterOptions{ - .path = "memory://closed.puffin", - .io = io, - .load_previous_deletes = NoPreviousDeletes})); + ICEBERG_UNWRAP_OR_FAIL(auto writer, DeletionVectorWriter::Make(MakeDVWriterOptions( + io, "memory://closed.puffin"))); ASSERT_THAT(writer->Close(), IsOk()); EXPECT_THAT(writer->Delete("data-a.parquet", 0, spec, PartitionValues{}), IsError(ErrorKind::kValidationFailed)); diff --git a/src/iceberg/test/endian_test.cc b/src/iceberg/test/endian_test.cc index 717baeafe..93aadde4e 100644 --- a/src/iceberg/test/endian_test.cc +++ b/src/iceberg/test/endian_test.cc @@ -94,6 +94,9 @@ TEST(EndianTest, BufferReadWriteRoundTrip) { WriteLittleEndian(int32_t{-1}, buf.data()); EXPECT_EQ(ReadLittleEndian(buf.data()), -1); + + WriteBigEndian(int32_t{0x12345678}, buf.data()); + EXPECT_EQ(ReadBigEndian(buf.data()), 0x12345678); } } // namespace iceberg diff --git a/src/iceberg/test/file_scan_task_reader_test.cc b/src/iceberg/test/file_scan_task_reader_test.cc index 59af21a93..09c89a4d4 100644 --- a/src/iceberg/test/file_scan_task_reader_test.cc +++ b/src/iceberg/test/file_scan_task_reader_test.cc @@ -42,6 +42,7 @@ #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" #include "iceberg/file_format.h" +#include "iceberg/file_io.h" #include "iceberg/file_reader.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/parquet/parquet_register.h" @@ -306,7 +307,7 @@ class FileScanTaskReaderTest : public TempFileTestBase { .path = path, .io = file_io_, .load_previous_deletes = [](std::string_view) - -> Result> { return nullptr; }, + -> Result> { return std::nullopt; }, }; ICEBERG_ASSIGN_OR_RAISE(auto writer, DeletionVectorWriter::Make(std::move(options))); for (int64_t pos : positions) { @@ -315,7 +316,7 @@ class FileScanTaskReaderTest : public TempFileTestBase { } ICEBERG_RETURN_UNEXPECTED(writer->Close()); ICEBERG_ASSIGN_OR_RAISE(auto metadata, writer->Metadata()); - return metadata.delete_files[0]; + return metadata.data_files[0]; } void VerifyStream(struct ArrowArrayStream* stream, std::string_view expected_json) { diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 2e07d273c..0e5399347 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -124,16 +124,13 @@ iceberg_tests = { }, 'puffin_test': { 'sources': files( + 'deletion_vector_writer_test.cc', 'puffin_format_test.cc', 'puffin_json_test.cc', 'puffin_reader_writer_test.cc', ), 'use_data': true, }, - 'deletion_vector_writer_test': { - 'sources': files('deletion_vector_writer_test.cc'), - 'use_data': true, - }, } if get_option('rest').enabled() diff --git a/src/iceberg/test/position_delete_index_test.cc b/src/iceberg/test/position_delete_index_test.cc index 124f06224..9b9fb01e7 100644 --- a/src/iceberg/test/position_delete_index_test.cc +++ b/src/iceberg/test/position_delete_index_test.cc @@ -349,4 +349,18 @@ TEST(PositionDeleteIndexTest, DeserializePreservesSourceDeleteFile) { EXPECT_EQ(restored.delete_files()[0], delete_file); } +TEST(PositionDeleteIndexTest, ConstructorsPreserveSourceDeleteFiles) { + auto file_a = std::make_shared(DataFile{.file_path = "a.parquet"}); + auto file_b = std::make_shared(DataFile{.file_path = "b.parquet"}); + + PositionDeleteIndex single(file_a); + ASSERT_EQ(single.delete_files().size(), 1u); + EXPECT_EQ(single.delete_files()[0], file_a); + + PositionDeleteIndex multiple(std::vector>{file_a, file_b}); + ASSERT_EQ(multiple.delete_files().size(), 2u); + EXPECT_EQ(multiple.delete_files()[0], file_a); + EXPECT_EQ(multiple.delete_files()[1], file_b); +} + } // namespace iceberg diff --git a/src/iceberg/util/endian.h b/src/iceberg/util/endian.h index 638273a1c..8aa559f40 100644 --- a/src/iceberg/util/endian.h +++ b/src/iceberg/util/endian.h @@ -112,4 +112,21 @@ T ReadLittleEndian(const void* input) { return FromLittleEndian(value); } +/// \brief Write a value in big-endian format to a buffer. +/// \note Caller must ensure output has at least sizeof(T) bytes available. +template +void WriteBigEndian(T value, void* output) { + auto be = ToBigEndian(value); + std::memcpy(output, &be, sizeof(be)); +} + +/// \brief Read a value in big-endian format from a buffer. +/// \note Caller must ensure input has at least sizeof(T) bytes available. +template +T ReadBigEndian(const void* input) { + T value; + std::memcpy(&value, input, sizeof(value)); + return FromBigEndian(value); +} + } // namespace iceberg diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index 123167324..8202e60a3 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -226,4 +226,11 @@ struct ICEBERG_EXPORT StringEqual { } }; +/// \brief Transparent less function that supports std::string_view as lookup key +struct ICEBERG_EXPORT StringLess { + using is_transparent = void; + + bool operator()(std::string_view lhs, std::string_view rhs) const { return lhs < rhs; } +}; + } // namespace iceberg diff --git a/src/iceberg/version.h.in b/src/iceberg/version.h.in index af7fe01e6..c7f8149fa 100644 --- a/src/iceberg/version.h.in +++ b/src/iceberg/version.h.in @@ -23,6 +23,8 @@ #define ICEBERG_VERSION_MINOR @PROJECT_VERSION_MINOR@ #define ICEBERG_VERSION_PATCH @PROJECT_VERSION_PATCH@ +#define ICEBERG_GIT_COMMIT_ID "@ICEBERG_GIT_COMMIT_ID@" #define ICEBERG_VERSION_STRING "@ICEBERG_VERSION_STRING@" +#define ICEBERG_FULL_VERSION_STRING "@ICEBERG_FULL_VERSION_STRING@" #define ICEBERG_PROJECT_NAME "@PROJECT_NAME@" From e4bb44b71ed7c1f2d3030e8d167cc5a0c3bedd84 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 14 Jul 2026 17:04:10 +0800 Subject: [PATCH 3/3] fix linter --- .clang-tidy | 2 +- src/iceberg/data/data_writer.cc | 2 ++ src/iceberg/data/deletion_vector_writer.cc | 11 ++++++++++- src/iceberg/data/dv_util.cc | 1 + src/iceberg/data/equality_delete_writer.cc | 2 ++ src/iceberg/data/position_delete_writer.cc | 2 ++ src/iceberg/deletes/position_delete_index.cc | 7 +++++++ src/iceberg/deletes/roaring_position_bitmap.cc | 2 ++ src/iceberg/test/delete_filter_test.cc | 2 ++ src/iceberg/test/delete_loader_test.cc | 3 +++ src/iceberg/test/deletion_vector_writer_test.cc | 4 ++++ src/iceberg/test/file_scan_task_reader_test.cc | 2 ++ src/iceberg/test/position_delete_index_test.cc | 2 ++ src/iceberg/util/gzip_internal.cc | 4 ++++ src/iceberg/util/string_util.cc | 6 ++++++ 15 files changed, 50 insertions(+), 2 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 5b13a0551..229bb0ec1 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -43,6 +43,6 @@ CheckOptions: - key: modernize-use-scoped-lock.WarnOnSingleLocks value: 'false' - key: misc-include-cleaner.IgnoreHeaders - value: 'arrow/.*;avro/.*;aws/.*;cpr/.*;gmock/.*;gtest/.*;nanoarrow/.*;nlohmann/.*;parquet/.*;roaring/.*;spdlog/.*;sqlpp23/.*;sqlite3\.h;thrift/.*;utf8proc\.h;zlib\.h' + value: 'arrow/.*;avro/.*;aws/.*;cpr/.*;gmock/.*;gtest/.*;nanoarrow/.*;nlohmann/.*;parquet/.*;roaring/.*;spdlog/.*;sqlpp23/.*;thrift/.*' HeaderFilterRegex: 'src/iceberg|example' diff --git a/src/iceberg/data/data_writer.cc b/src/iceberg/data/data_writer.cc index a63f05530..512f77cc9 100644 --- a/src/iceberg/data/data_writer.cc +++ b/src/iceberg/data/data_writer.cc @@ -20,7 +20,9 @@ #include "iceberg/data/data_writer.h" #include +#include +#include "iceberg/data/writer.h" // IWYU pragma: keep #include "iceberg/file_writer.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/partition_spec.h" diff --git a/src/iceberg/data/deletion_vector_writer.cc b/src/iceberg/data/deletion_vector_writer.cc index 27a84a022..8ff890627 100644 --- a/src/iceberg/data/deletion_vector_writer.cc +++ b/src/iceberg/data/deletion_vector_writer.cc @@ -19,20 +19,29 @@ #include "iceberg/data/deletion_vector_writer.h" +#include #include #include +#include #include #include +#include #include #include #include -#include "iceberg/file_io.h" +#include "iceberg/data/writer.h" +#include "iceberg/deletes/position_delete_index.h" +#include "iceberg/deletes/roaring_position_bitmap.h" +#include "iceberg/file_format.h" +#include "iceberg/file_io.h" // IWYU pragma: keep #include "iceberg/manifest/manifest_entry.h" #include "iceberg/metadata_columns.h" #include "iceberg/partition_spec.h" #include "iceberg/puffin/file_metadata.h" #include "iceberg/puffin/puffin_writer.h" +#include "iceberg/result.h" +#include "iceberg/row/partition_values.h" #include "iceberg/util/content_file_util.h" #include "iceberg/util/macros.h" #include "iceberg/util/string_util.h" diff --git a/src/iceberg/data/dv_util.cc b/src/iceberg/data/dv_util.cc index b549197ca..175644841 100644 --- a/src/iceberg/data/dv_util.cc +++ b/src/iceberg/data/dv_util.cc @@ -28,6 +28,7 @@ #include "iceberg/deletes/position_delete_index.h" #include "iceberg/file_io.h" #include "iceberg/manifest/manifest_entry.h" +#include "iceberg/result.h" #include "iceberg/util/content_file_util.h" #include "iceberg/util/macros.h" diff --git a/src/iceberg/data/equality_delete_writer.cc b/src/iceberg/data/equality_delete_writer.cc index 89e6485b0..86293addb 100644 --- a/src/iceberg/data/equality_delete_writer.cc +++ b/src/iceberg/data/equality_delete_writer.cc @@ -20,7 +20,9 @@ #include "iceberg/data/equality_delete_writer.h" #include +#include +#include "iceberg/data/writer.h" // IWYU pragma: keep #include "iceberg/file_writer.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/partition_spec.h" diff --git a/src/iceberg/data/position_delete_writer.cc b/src/iceberg/data/position_delete_writer.cc index 8c0e7a58a..e487e0e97 100644 --- a/src/iceberg/data/position_delete_writer.cc +++ b/src/iceberg/data/position_delete_writer.cc @@ -21,12 +21,14 @@ #include #include +#include #include #include #include "iceberg/arrow/nanoarrow_status_internal.h" #include "iceberg/arrow_c_data_guard_internal.h" +#include "iceberg/data/writer.h" // IWYU pragma: keep #include "iceberg/file_writer.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/metadata_columns.h" diff --git a/src/iceberg/deletes/position_delete_index.cc b/src/iceberg/deletes/position_delete_index.cc index 0d70cca4c..53e33e635 100644 --- a/src/iceberg/deletes/position_delete_index.cc +++ b/src/iceberg/deletes/position_delete_index.cc @@ -19,16 +19,23 @@ #include "iceberg/deletes/position_delete_index.h" +#include #include #include #include +#include #include #include +#include +#include #include #include +#include +#include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/manifest/manifest_entry.h" +#include "iceberg/result.h" #include "iceberg/util/endian.h" #include "iceberg/util/macros.h" diff --git a/src/iceberg/deletes/roaring_position_bitmap.cc b/src/iceberg/deletes/roaring_position_bitmap.cc index 93624d2bf..a2827d4bb 100644 --- a/src/iceberg/deletes/roaring_position_bitmap.cc +++ b/src/iceberg/deletes/roaring_position_bitmap.cc @@ -19,9 +19,11 @@ #include "iceberg/deletes/roaring_position_bitmap.h" +#include #include #include #include +#include #include #include diff --git a/src/iceberg/test/delete_filter_test.cc b/src/iceberg/test/delete_filter_test.cc index d4ef94960..4b5e888e5 100644 --- a/src/iceberg/test/delete_filter_test.cc +++ b/src/iceberg/test/delete_filter_test.cc @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -37,6 +38,7 @@ #include "iceberg/data/deletion_vector_writer.h" #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" +#include "iceberg/deletes/position_delete_index.h" #include "iceberg/file_format.h" #include "iceberg/file_io.h" #include "iceberg/file_reader.h" diff --git a/src/iceberg/test/delete_loader_test.cc b/src/iceberg/test/delete_loader_test.cc index db166875d..a5dda22e9 100644 --- a/src/iceberg/test/delete_loader_test.cc +++ b/src/iceberg/test/delete_loader_test.cc @@ -20,6 +20,7 @@ #include "iceberg/data/delete_loader.h" #include +#include #include #include @@ -37,12 +38,14 @@ #include "iceberg/manifest/manifest_entry.h" #include "iceberg/parquet/parquet_register.h" #include "iceberg/partition_spec.h" +#include "iceberg/result.h" #include "iceberg/row/partition_values.h" #include "iceberg/schema.h" #include "iceberg/schema_field.h" #include "iceberg/schema_internal.h" #include "iceberg/test/matchers.h" #include "iceberg/type.h" +#include "iceberg/util/macros.h" #include "iceberg/util/struct_like_set.h" namespace iceberg { diff --git a/src/iceberg/test/deletion_vector_writer_test.cc b/src/iceberg/test/deletion_vector_writer_test.cc index 7e3cf050f..605b2d4e1 100644 --- a/src/iceberg/test/deletion_vector_writer_test.cc +++ b/src/iceberg/test/deletion_vector_writer_test.cc @@ -19,10 +19,12 @@ #include "iceberg/data/deletion_vector_writer.h" +#include #include #include #include #include +#include #include #include @@ -37,9 +39,11 @@ #include "iceberg/partition_spec.h" #include "iceberg/puffin/file_metadata.h" #include "iceberg/puffin/puffin_reader.h" +#include "iceberg/result.h" #include "iceberg/row/partition_values.h" #include "iceberg/test/matchers.h" #include "iceberg/test/mock_io.h" +#include "iceberg/util/macros.h" #include "iceberg/version.h" namespace iceberg { diff --git a/src/iceberg/test/file_scan_task_reader_test.cc b/src/iceberg/test/file_scan_task_reader_test.cc index 09c89a4d4..10e9c8e5f 100644 --- a/src/iceberg/test/file_scan_task_reader_test.cc +++ b/src/iceberg/test/file_scan_task_reader_test.cc @@ -36,11 +36,13 @@ #include "iceberg/arrow/arrow_io_internal.h" #include "iceberg/arrow/arrow_register.h" +#include "iceberg/arrow_c_data.h" #include "iceberg/arrow_c_data_guard_internal.h" #include "iceberg/arrow_c_data_util_internal.h" #include "iceberg/data/deletion_vector_writer.h" #include "iceberg/data/equality_delete_writer.h" #include "iceberg/data/position_delete_writer.h" +#include "iceberg/deletes/position_delete_index.h" #include "iceberg/file_format.h" #include "iceberg/file_io.h" #include "iceberg/file_reader.h" diff --git a/src/iceberg/test/position_delete_index_test.cc b/src/iceberg/test/position_delete_index_test.cc index 9b9fb01e7..1075ee6fa 100644 --- a/src/iceberg/test/position_delete_index_test.cc +++ b/src/iceberg/test/position_delete_index_test.cc @@ -25,7 +25,9 @@ #include +#include "iceberg/file_format.h" #include "iceberg/manifest/manifest_entry.h" +#include "iceberg/result.h" #include "iceberg/test/matchers.h" namespace iceberg { diff --git a/src/iceberg/util/gzip_internal.cc b/src/iceberg/util/gzip_internal.cc index 680e2bc33..63e5acb2a 100644 --- a/src/iceberg/util/gzip_internal.cc +++ b/src/iceberg/util/gzip_internal.cc @@ -19,11 +19,15 @@ #include "iceberg/util/gzip_internal.h" +#include #include #include +#include +#include #include +#include "iceberg/result.h" #include "iceberg/util/macros.h" namespace iceberg { diff --git a/src/iceberg/util/string_util.cc b/src/iceberg/util/string_util.cc index 8b98397db..b4c717861 100644 --- a/src/iceberg/util/string_util.cc +++ b/src/iceberg/util/string_util.cc @@ -22,7 +22,13 @@ #include #include +#include +#include +#include +#include +#include +#include "iceberg/result.h" #include "iceberg/util/macros.h" namespace iceberg {