From a53c5f22551664f50db5f9f0899984e5472c2ebe Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Tue, 23 Jun 2026 23:44:45 +0800 Subject: [PATCH 1/3] feat: add rewrite manifests --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/cpp20_compatibility_internal.h | 1 + src/iceberg/manifest/manifest_util.cc | 31 + src/iceberg/manifest/manifest_util_internal.h | 8 + src/iceberg/manifest/manifest_writer.cc | 9 +- src/iceberg/manifest/manifest_writer.h | 2 + src/iceberg/manifest/v3_metadata.cc | 13 +- src/iceberg/table.cc | 11 + src/iceberg/table.h | 5 + src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/rewrite_manifests_test.cc | 2385 +++++++++++++++++ src/iceberg/test/update_test_base.h | 113 + src/iceberg/transaction.cc | 8 + src/iceberg/transaction.h | 3 + src/iceberg/type_fwd.h | 1 + src/iceberg/update/rewrite_manifests.cc | 443 +++ src/iceberg/update/rewrite_manifests.h | 143 + src/iceberg/update/snapshot_update.h | 9 + 18 files changed, 3183 insertions(+), 4 deletions(-) create mode 100644 src/iceberg/test/rewrite_manifests_test.cc create mode 100644 src/iceberg/update/rewrite_manifests.cc create mode 100644 src/iceberg/update/rewrite_manifests.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 7394ce82c6..e2ec26461f 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -129,6 +129,7 @@ set(ICEBERG_SOURCES update/overwrite_files.cc update/pending_update.cc update/rewrite_files.cc + update/rewrite_manifests.cc update/row_delta.cc update/set_snapshot.cc update/snapshot_manager.cc diff --git a/src/iceberg/cpp20_compatibility_internal.h b/src/iceberg/cpp20_compatibility_internal.h index 8c4a465f03..31f229579c 100644 --- a/src/iceberg/cpp20_compatibility_internal.h +++ b/src/iceberg/cpp20_compatibility_internal.h @@ -149,6 +149,7 @@ #include "iceberg/update/pending_update.h" #include "iceberg/update/replace_partitions.h" #include "iceberg/update/rewrite_files.h" +#include "iceberg/update/rewrite_manifests.h" #include "iceberg/update/row_delta.h" #include "iceberg/update/set_snapshot.h" #include "iceberg/update/snapshot_manager.h" diff --git a/src/iceberg/manifest/manifest_util.cc b/src/iceberg/manifest/manifest_util.cc index 12805452d4..c6fe4d453f 100644 --- a/src/iceberg/manifest/manifest_util.cc +++ b/src/iceberg/manifest/manifest_util.cc @@ -68,4 +68,35 @@ Result CopyAppendManifest( return writer->ToManifestFile(); } +Result CopyRewriteManifest( + const ManifestFile& manifest, const std::shared_ptr& file_io, + const std::shared_ptr& schema, const std::shared_ptr& spec, + int64_t snapshot_id, const std::string& output_path, int8_t format_version) { + // Require explicit snapshot IDs and preserve them in the copied entries. + ICEBERG_ASSIGN_OR_RAISE(auto inheritable_metadata, InheritableMetadataFactory::Empty()); + ICEBERG_ASSIGN_OR_RAISE( + auto reader, ManifestReader::Make( + manifest.manifest_path, manifest.manifest_length, file_io, schema, + spec, std::move(inheritable_metadata), manifest.first_row_id, + /*is_committed=*/false)); + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->Entries()); + + ICEBERG_ASSIGN_OR_RAISE( + auto writer, + ManifestWriter::MakeWriter(format_version, snapshot_id, output_path, file_io, spec, + schema, manifest.content, manifest.first_row_id)); + for (const auto& entry : entries) { + // A rewritten added manifest may only contain existing entries. + if (entry.status == ManifestStatus::kAdded) { + return ValidationFailed("Cannot add manifest with added files"); + } + if (entry.status == ManifestStatus::kDeleted) { + return ValidationFailed("Cannot add manifest with deleted files"); + } + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + return writer->ToManifestFile(); +} + } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_util_internal.h b/src/iceberg/manifest/manifest_util_internal.h index e68437d08e..77de2aaa12 100644 --- a/src/iceberg/manifest/manifest_util_internal.h +++ b/src/iceberg/manifest/manifest_util_internal.h @@ -53,4 +53,12 @@ ICEBERG_EXPORT Result CopyAppendManifest( int64_t snapshot_id, const std::string& output_path, int8_t format_version, SnapshotSummaryBuilder* summary_builder = nullptr); +/// \brief Copy a manifest containing only EXISTING entries, preserving their +/// snapshot IDs, sequence numbers, and row IDs. The caller owns output_path and +/// must clean it up if copying or committing fails. +ICEBERG_EXPORT Result CopyRewriteManifest( + const ManifestFile& manifest, const std::shared_ptr& file_io, + const std::shared_ptr& schema, const std::shared_ptr& spec, + int64_t snapshot_id, const std::string& output_path, int8_t format_version); + } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_writer.cc b/src/iceberg/manifest/manifest_writer.cc index bdc6ba2a16..d7a7274116 100644 --- a/src/iceberg/manifest/manifest_writer.cc +++ b/src/iceberg/manifest/manifest_writer.cc @@ -31,6 +31,7 @@ #include "iceberg/result.h" #include "iceberg/schema.h" #include "iceberg/table_metadata.h" +#include "iceberg/util/error_util_internal.h" #include "iceberg/util/macros.h" namespace iceberg { @@ -46,7 +47,13 @@ ManifestWriter::ManifestWriter(std::unique_ptr writer, partition_summary_( std::make_unique(*adapter_->partition_type())) {} -ManifestWriter::~ManifestWriter() = default; +ManifestWriter::~ManifestWriter() { + if (!closed_) { + // Release the output on failure without flushing uncommitted adapter entries. + internal::LogAndIgnoreFailure("Manifest writer cleanup", + [this] { return writer_->Close(); }); + } +} Status ManifestWriter::WriteAddedEntry(std::shared_ptr file, std::optional data_sequence_number) { diff --git a/src/iceberg/manifest/manifest_writer.h b/src/iceberg/manifest/manifest_writer.h index d8d9afdb74..a5ffcb3ddc 100644 --- a/src/iceberg/manifest/manifest_writer.h +++ b/src/iceberg/manifest/manifest_writer.h @@ -39,6 +39,8 @@ namespace iceberg { /// \brief Write manifest entries to a manifest file. class ICEBERG_EXPORT ManifestWriter { public: + /// Close the underlying output on destruction. Call Close() explicitly to flush + /// all entries and report write or close errors before committing the manifest. ~ManifestWriter(); /// \brief Write the entry that all its fields are populated correctly. diff --git a/src/iceberg/manifest/v3_metadata.cc b/src/iceberg/manifest/v3_metadata.cc index ad577b240d..504768c588 100644 --- a/src/iceberg/manifest/v3_metadata.cc +++ b/src/iceberg/manifest/v3_metadata.cc @@ -194,13 +194,20 @@ Status ManifestFileAdapterV3::Init() { } Status ManifestFileAdapterV3::Append(const ManifestFile& file) { - ICEBERG_RETURN_UNEXPECTED(AppendInternal(file)); if (WrapFirstRowId(file)) { if (!next_row_id_.has_value()) { return InvalidManifestList("Missing next-row-id for file: {}", file.manifest_path); } - next_row_id_ = next_row_id_.value() + file.existing_rows_count.value_or(0) + - file.added_rows_count.value_or(0); + if (!file.existing_rows_count.has_value() || !file.added_rows_count.has_value()) { + return InvalidManifestList("Missing row counts for file: {}", file.manifest_path); + } + } + + ICEBERG_RETURN_UNEXPECTED(AppendInternal(file)); + + if (WrapFirstRowId(file)) { + next_row_id_ = next_row_id_.value() + file.existing_rows_count.value() + + file.added_rows_count.value(); } return {}; } diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index c512c11dfb..d9b209fe26 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -38,6 +38,7 @@ #include "iceberg/update/overwrite_files.h" #include "iceberg/update/replace_partitions.h" #include "iceberg/update/rewrite_files.h" +#include "iceberg/update/rewrite_manifests.h" #include "iceberg/update/row_delta.h" #include "iceberg/update/set_snapshot.h" #include "iceberg/update/snapshot_manager.h" @@ -266,6 +267,12 @@ Result> Table::NewReplacePartitions() { return ReplacePartitions::Make(name().name, std::move(ctx)); } +Result> Table::NewRewriteManifests() { + ICEBERG_ASSIGN_OR_RAISE( + auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); + return RewriteManifests::Make(name().name, std::move(ctx)); +} + Result> Table::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE( auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); @@ -390,6 +397,10 @@ Result> StaticTable::NewReplacePartitions() { return NotSupported("Cannot replace partitions for a static table"); } +Result> StaticTable::NewRewriteManifests() { + return NotSupported("Cannot create a rewrite manifests for a static table"); +} + Result> StaticTable::NewSnapshotManager() { return NotSupported("Cannot create a snapshot manager for a static table"); } diff --git a/src/iceberg/table.h b/src/iceberg/table.h index 4c0470cb9c..f277d0961e 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -209,6 +209,9 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this { /// the changes. virtual Result> NewReplacePartitions(); + /// \brief Create a new RewriteManifests to rewrite manifest layout. + virtual Result> NewRewriteManifests(); + /// \brief Create a new SnapshotManager to manage snapshots and snapshot references. virtual Result> NewSnapshotManager(); @@ -293,6 +296,8 @@ class ICEBERG_EXPORT StaticTable : public Table { Result> NewReplacePartitions() override; + Result> NewRewriteManifests() override; + Result> NewSnapshotManager() override; private: diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 058a9dd3cc..0a8bcbf22c 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -243,6 +243,7 @@ if(ICEBERG_BUILD_BUNDLE) merging_snapshot_update_test.cc name_mapping_update_test.cc rewrite_files_test.cc + rewrite_manifests_test.cc row_delta_test.cc snapshot_manager_test.cc transaction_test.cc diff --git a/src/iceberg/test/rewrite_manifests_test.cc b/src/iceberg/test/rewrite_manifests_test.cc new file mode 100644 index 0000000000..b68f0077bf --- /dev/null +++ b/src/iceberg/test/rewrite_manifests_test.cc @@ -0,0 +1,2385 @@ +/* + * 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/update/rewrite_manifests.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "iceberg/avro/avro_register.h" +#include "iceberg/constants.h" +#include "iceberg/expression/expressions.h" +#include "iceberg/file_io.h" +#include "iceberg/partition_spec.h" +#include "iceberg/schema.h" +#include "iceberg/table_metadata.h" +#include "iceberg/table_properties.h" +#include "iceberg/test/executor.h" +#include "iceberg/test/mock_catalog.h" +#include "iceberg/test/update_test_base.h" +#include "iceberg/transaction.h" +#include "iceberg/transform.h" +#include "iceberg/update/delete_files.h" +#include "iceberg/update/fast_append.h" +#include "iceberg/update/row_delta.h" +#include "iceberg/update/update_partition_spec.h" +#include "iceberg/update/update_properties.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +namespace { + +struct ManifestWriteFailureState { + int manifest_output_count = 0; + int fail_on_manifest_output = 0; + int manifest_close_count = 0; + int fail_on_manifest_close = 0; + int throw_on_manifest_close = 0; + std::vector manifest_outputs; + std::vector manifest_close_attempts; + std::vector deleted_files; + std::unordered_set open_manifest_paths; + std::vector deleted_open_files; +}; + +class ManifestWriteFailureOutputStream : public PositionOutputStream { + public: + ManifestWriteFailureOutputStream(std::unique_ptr delegate, + std::string location, + std::shared_ptr state) + : delegate_(std::move(delegate)), + location_(std::move(location)), + state_(std::move(state)) { + state_->open_manifest_paths.insert(location_); + } + + Result Position() const override { return delegate_->Position(); } + + Result StoredLength() const override { return delegate_->StoredLength(); } + + Status Write(std::span data) override { + return delegate_->Write(data); + } + + Status Flush() override { return delegate_->Flush(); } + + Status Close() override { + if (closed_) { + return {}; + } + closed_ = true; + state_->manifest_close_attempts.push_back(location_); + ++state_->manifest_close_count; + ICEBERG_RETURN_UNEXPECTED(delegate_->Close()); + state_->open_manifest_paths.erase(location_); + if (state_->manifest_close_count == state_->throw_on_manifest_close) { + throw std::runtime_error("Injected rewrite manifest close exception"); + } + if (state_->manifest_close_count == state_->fail_on_manifest_close) { + return IOError("Injected rewrite manifest close failure"); + } + return {}; + } + + private: + std::unique_ptr delegate_; + std::string location_; + std::shared_ptr state_; + bool closed_{false}; +}; + +class ManifestWriteFailureOutputFile : public OutputFile { + public: + ManifestWriteFailureOutputFile(std::unique_ptr delegate, + std::string location, + std::shared_ptr state) + : delegate_(std::move(delegate)), + location_(std::move(location)), + state_(std::move(state)) {} + + std::string_view location() const override { return location_; } + + Result> Create() override { + ICEBERG_ASSIGN_OR_RAISE(auto stream, delegate_->Create()); + return std::make_unique(std::move(stream), + location_, state_); + } + + Result> CreateOrOverwrite() override { + ICEBERG_ASSIGN_OR_RAISE(auto stream, delegate_->CreateOrOverwrite()); + return std::make_unique(std::move(stream), + location_, state_); + } + + private: + std::unique_ptr delegate_; + std::string location_; + std::shared_ptr state_; +}; + +class ManifestWriteFailureFileIO : public FileIO { + public: + ManifestWriteFailureFileIO(std::shared_ptr delegate, + std::string metadata_location, + std::shared_ptr state) + : delegate_(std::move(delegate)), + metadata_location_(std::move(metadata_location)), + state_(std::move(state)) {} + + Result> NewInputFile(std::string file_location) override { + return delegate_->NewInputFile(std::move(file_location)); + } + + Result> NewInputFile(std::string file_location, + size_t length) override { + return delegate_->NewInputFile(std::move(file_location), length); + } + + Result> NewOutputFile(std::string file_location) override { + const bool is_manifest_output = IsManifestOutput(file_location); + if (is_manifest_output) { + state_->manifest_outputs.push_back(file_location); + ++state_->manifest_output_count; + if (state_->manifest_output_count == state_->fail_on_manifest_output) { + return IOError("Injected rewrite manifest writer failure"); + } + } + ICEBERG_ASSIGN_OR_RAISE(auto output_file, delegate_->NewOutputFile(file_location)); + if (is_manifest_output) { + return std::make_unique( + std::move(output_file), std::move(file_location), state_); + } + return output_file; + } + + Status DeleteFile(const std::string& file_location) override { + if (state_->open_manifest_paths.contains(file_location)) { + state_->deleted_open_files.push_back(file_location); + } + state_->deleted_files.push_back(file_location); + return delegate_->DeleteFile(file_location); + } + + Status DeleteFiles(const std::vector& file_locations) override { + state_->deleted_files.insert(state_->deleted_files.end(), file_locations.begin(), + file_locations.end()); + return delegate_->DeleteFiles(file_locations); + } + + private: + bool IsManifestOutput(const std::string& file_location) const { + return file_location.starts_with(metadata_location_) && + file_location.ends_with(".avro"); + } + + std::shared_ptr delegate_; + std::string metadata_location_; + std::shared_ptr state_; +}; + +} // namespace + +class RewriteManifestsTest : public MinimalUpdateTestBase, + public ::testing::WithParamInterface { + protected: + static void SetUpTestSuite() { avro::RegisterAll(); } + + void SetUp() override { + MinimalUpdateTestBase::SetUp(); + + ICEBERG_UNWRAP_OR_FAIL(spec_, table_->spec()); + ICEBERG_UNWRAP_OR_FAIL(schema_, table_->schema()); + file_a_ = MakeDataFile("a", 10, 100, 1); + file_b_ = MakeDataFile("b", 20, 200, 2); + file_c_ = MakeDataFile("c", 30, 300, 3); + file_d_ = MakeDataFile("d", 40, 400, 4); + } + + int8_t format_version() const override { return GetParam(); } + + std::shared_ptr MakeDataFile(const std::string& name, int64_t record_count, + int64_t size, int64_t partition_value) { + return MakeDataFile(name, spec_, record_count, size, + {Literal::Long(partition_value)}); + } + + std::shared_ptr MakeDataFile(const std::string& name, + std::shared_ptr spec, + int64_t record_count, int64_t size, + std::vector partition_values) { + auto file = std::make_shared(); + file->content = DataFile::Content::kData; + file->file_path = std::format("{}/data/{}.parquet", table_location_, name); + file->file_format = FileFormatType::kParquet; + file->partition = PartitionValues(std::move(partition_values)); + file->record_count = record_count; + file->file_size_in_bytes = size; + file->partition_spec_id = spec->spec_id(); + return file; + } + + std::shared_ptr MakeDeleteFile( + const std::string& name, int64_t partition_value, + std::optional referenced_data_file = std::nullopt) { + auto file = MakeDataFile(name, 7, 50, partition_value); + file->content = DataFile::Content::kPositionDeletes; + file->file_path = std::format("{}/delete/{}.parquet", table_location_, name); + if (table_->metadata()->format_version >= 3) { + file->file_format = FileFormatType::kPuffin; + file->referenced_data_file = referenced_data_file.value_or(file_a_->file_path); + file->content_offset = 0; + file->content_size_in_bytes = 10; + } + return file; + } + + std::shared_ptr MakeEqualityDeleteFile(const std::string& name, + int64_t partition_value) { + auto file = MakeDataFile(name, /*record_count=*/1, /*size=*/10, partition_value); + file->content = DataFile::Content::kEqualityDeletes; + file->file_path = std::format("{}/delete/{}.parquet", table_location_, name); + file->equality_ids = {1}; + return file; + } + + Status AppendFiles(std::vector> files) { + ICEBERG_ASSIGN_OR_RAISE(auto append, table_->NewFastAppend()); + for (const auto& file : files) { + append->AppendFile(file); + } + ICEBERG_RETURN_UNEXPECTED(append->Commit()); + return table_->Refresh(); + } + + Status AppendDeleteFiles(std::vector> files) { + ICEBERG_ASSIGN_OR_RAISE(auto row_delta, table_->NewRowDelta()); + for (const auto& file : files) { + row_delta->AddDeletes(file); + } + ICEBERG_RETURN_UNEXPECTED(row_delta->Commit()); + return table_->Refresh(); + } + + Status RemoveDeleteFiles(std::vector> files) { + ICEBERG_ASSIGN_OR_RAISE(auto row_delta, table_->NewRowDelta()); + for (const auto& file : files) { + row_delta->RemoveDeletes(file); + } + ICEBERG_RETURN_UNEXPECTED(row_delta->Commit()); + return table_->Refresh(); + } + + void SetSnapshotIdInheritanceEnabled() { + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kSnapshotIdInheritanceEnabled.key()), "true"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + void SetManifestMergeEnabled(bool enabled) { + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kManifestMergeEnabled.key()), + enabled ? "true" : "false"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + void SetCommitRetryProperties(int32_t retries) { + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kCommitNumRetries.key()), + std::to_string(retries)); + props->Set(std::string(TableProperties::kCommitMinRetryWaitMs.key()), "1"); + props->Set(std::string(TableProperties::kCommitMaxRetryWaitMs.key()), "1"); + props->Set(std::string(TableProperties::kCommitTotalRetryTimeMs.key()), "1000"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + void BindTableWithFailingCommits(int failures) { + auto mock_catalog = std::make_shared<::testing::NiceMock>(); + std::weak_ptr<::testing::NiceMock> weak_catalog = mock_catalog; + + ON_CALL(*mock_catalog, LoadTable(::testing::_)) + .WillByDefault([this, weak_catalog](const TableIdentifier& identifier) + -> Result> { + ICEBERG_ASSIGN_OR_RAISE(auto loaded, catalog_->LoadTable(identifier)); + auto catalog = weak_catalog.lock(); + ICEBERG_PRECHECK(catalog != nullptr, "Mock catalog expired"); + return Table::Make(loaded->name(), loaded->metadata(), + std::string(loaded->metadata_file_location()), loaded->io(), + catalog); + }); + + ON_CALL(*mock_catalog, UpdateTable(::testing::_, ::testing::_, ::testing::_)) + .WillByDefault( + [this, weak_catalog, failures]( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates) mutable + -> Result> { + if (failures-- > 0) { + return CommitFailed("Injected failure"); + } + ICEBERG_ASSIGN_OR_RAISE( + auto updated, catalog_->UpdateTable(identifier, requirements, updates)); + auto catalog = weak_catalog.lock(); + ICEBERG_PRECHECK(catalog != nullptr, "Mock catalog expired"); + return Table::Make(updated->name(), updated->metadata(), + std::string(updated->metadata_file_location()), + updated->io(), catalog); + }); + + ICEBERG_UNWRAP_OR_FAIL(auto bound_table, + Table::Make(table_->name(), table_->metadata(), + std::string(table_->metadata_file_location()), + table_->io(), mock_catalog)); + table_ = std::move(bound_table); + mock_catalog_ = std::move(mock_catalog); + } + + void BindTableWithFileIO(std::shared_ptr file_io) { + ICEBERG_UNWRAP_OR_FAIL(auto bound_table, + Table::Make(table_->name(), table_->metadata(), + std::string(table_->metadata_file_location()), + std::move(file_io), catalog_)); + table_ = std::move(bound_table); + } + + void ValidateSummary(const std::shared_ptr& snapshot, int replaced, int kept, + int created, int entry_count) { + ASSERT_NE(snapshot, nullptr); + const auto& summary = snapshot->summary; + EXPECT_THAT(summary, ::testing::Contains(::testing::Pair( + std::string(SnapshotSummaryFields::kManifestsReplaced), + std::to_string(replaced)))); + EXPECT_THAT(summary, ::testing::Contains(::testing::Pair( + std::string(SnapshotSummaryFields::kManifestsKept), + std::to_string(kept)))); + EXPECT_THAT(summary, ::testing::Contains(::testing::Pair( + std::string(SnapshotSummaryFields::kManifestsCreated), + std::to_string(created)))); + EXPECT_THAT(summary, ::testing::Contains(::testing::Pair( + std::string(SnapshotSummaryFields::kEntriesProcessed), + std::to_string(entry_count)))); + } + + void MatchNumberOfManifestFileWithSpecId(std::span manifests, + int32_t partition_spec_id, + int expected_manifest_count) { + const auto matched_manifest_count = std::ranges::count_if( + manifests, [partition_spec_id](const ManifestFile& manifest) { + return manifest.partition_spec_id == partition_spec_id; + }); + EXPECT_EQ(matched_manifest_count, expected_manifest_count); + } + + bool FileExists(const std::string& path) { + auto input_file = file_io_->NewInputFile(path); + if (!input_file.has_value()) { + return false; + } + return input_file.value()->Size().has_value(); + } + + Result ManifestContainingPath( + std::span manifests, const std::string& path) { + for (const auto& manifest : manifests) { + ICEBERG_ASSIGN_OR_RAISE(auto entries, ReadManifestEntries(manifest)); + for (const auto& entry : entries) { + if (entry.data_file != nullptr && entry.data_file->file_path == path) { + return &manifest; + } + } + } + return NotFound("Manifest containing path {} is not found", path); + } + + void ExpectManifestEntryContents(const ManifestFile& manifest, + std::vector expected_paths, + std::vector expected_contents) { + ASSERT_EQ(expected_contents.size(), expected_paths.size()); + + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + ASSERT_EQ(entries.size(), expected_paths.size()); + + std::vector> actual; + actual.reserve(entries.size()); + for (const auto& entry : entries) { + ASSERT_NE(entry.data_file, nullptr); + actual.emplace_back(entry.data_file->file_path, entry.data_file->content); + } + + std::vector> expected; + expected.reserve(expected_paths.size()); + for (size_t i = 0; i < expected_paths.size(); ++i) { + expected.emplace_back(std::move(expected_paths[i]), expected_contents[i]); + } + EXPECT_THAT(actual, ::testing::UnorderedElementsAreArray(expected)); + } + + Result> CurrentManifests() { + ICEBERG_RETURN_UNEXPECTED(table_->Refresh()); + ICEBERG_ASSIGN_OR_RAISE(auto snapshot, table_->current_snapshot()); + SnapshotReader reader(snapshot.get()); + ICEBERG_ASSIGN_OR_RAISE(auto manifests, reader.DataManifests(file_io_)); + return std::vector(manifests.begin(), manifests.end()); + } + + Result> CurrentDeleteManifests() { + ICEBERG_RETURN_UNEXPECTED(table_->Refresh()); + ICEBERG_ASSIGN_OR_RAISE(auto snapshot, table_->current_snapshot()); + SnapshotReader reader(snapshot.get()); + ICEBERG_ASSIGN_OR_RAISE(auto manifests, reader.DeleteManifests(file_io_)); + return std::vector(manifests.begin(), manifests.end()); + } + + Result WriteExistingManifest( + const std::string& name, int64_t snapshot_id, + const std::vector>& files, + ManifestContent content = ManifestContent::kData, + std::shared_ptr manifest_spec = nullptr, + int64_t data_sequence_number = TableMetadata::kInitialSequenceNumber, + std::optional file_sequence_number = std::nullopt) { + auto path = std::format("{}/metadata/{}.avro", table_location_, name); + auto spec = manifest_spec != nullptr ? std::move(manifest_spec) : spec_; + std::vector> files_to_write; + files_to_write.reserve(files.size()); + std::optional first_row_id = std::nullopt; + if (table_->metadata()->format_version >= 3 && content == ManifestContent::kData) { + first_row_id = TableMetadata::kInitialRowId; + ICEBERG_ASSIGN_OR_RAISE(auto current_manifests, CurrentManifests()); + std::unordered_map> current_files_by_path; + for (const auto& manifest : current_manifests) { + ICEBERG_ASSIGN_OR_RAISE(auto entries, ReadManifestEntries(manifest)); + for (const auto& entry : entries) { + if (entry.IsAlive() && entry.data_file != nullptr) { + current_files_by_path.emplace(entry.data_file->file_path, + std::make_shared(*entry.data_file)); + } + } + } + for (const auto& file : files) { + auto file_to_write = file; + if (!file_to_write->first_row_id.has_value()) { + if (auto it = current_files_by_path.find(file->file_path); + it != current_files_by_path.end()) { + file_to_write = it->second; + } + } + files_to_write.push_back(std::move(file_to_write)); + } + } else { + files_to_write = files; + } + ICEBERG_ASSIGN_OR_RAISE( + auto writer, + ManifestWriter::MakeWriter(table_->metadata()->format_version, kInvalidSnapshotId, + path, file_io_, spec, schema_, content, first_row_id)); + for (const auto& file : files_to_write) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry( + file, snapshot_id, data_sequence_number, file_sequence_number)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + return writer->ToManifestFile(); + } + + struct ExistingManifestEntry { + std::shared_ptr file; + int64_t snapshot_id; + int64_t data_sequence_number; + int64_t file_sequence_number; + }; + + Result WriteExistingDeleteManifest( + const std::string& name, const std::vector& entries) { + auto path = std::format("{}/metadata/{}.avro", table_location_, name); + ICEBERG_ASSIGN_OR_RAISE( + auto writer, ManifestWriter::MakeWriter(table_->metadata()->format_version, + kInvalidSnapshotId, path, file_io_, spec_, + schema_, ManifestContent::kDeletes)); + for (const auto& entry : entries) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry.file, entry.snapshot_id, + entry.data_sequence_number, + entry.file_sequence_number)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + return writer->ToManifestFile(); + } + + Result WriteExistingManifest( + const std::string& name, + const std::vector, int64_t>>& files) { + auto path = std::format("{}/metadata/{}.avro", table_location_, name); + std::vector, int64_t>> files_to_write; + files_to_write.reserve(files.size()); + std::optional first_row_id = std::nullopt; + if (table_->metadata()->format_version >= 3) { + first_row_id = TableMetadata::kInitialRowId; + ICEBERG_ASSIGN_OR_RAISE(auto current_manifests, CurrentManifests()); + std::unordered_map> current_files_by_path; + for (const auto& manifest : current_manifests) { + ICEBERG_ASSIGN_OR_RAISE(auto entries, ReadManifestEntries(manifest)); + for (const auto& entry : entries) { + if (entry.IsAlive() && entry.data_file != nullptr) { + current_files_by_path.emplace(entry.data_file->file_path, + std::make_shared(*entry.data_file)); + } + } + } + for (const auto& [file, snapshot_id] : files) { + auto file_to_write = file; + if (!file_to_write->first_row_id.has_value()) { + if (auto it = current_files_by_path.find(file->file_path); + it != current_files_by_path.end()) { + file_to_write = it->second; + } + } + files_to_write.emplace_back(std::move(file_to_write), snapshot_id); + } + } else { + files_to_write = files; + } + ICEBERG_ASSIGN_OR_RAISE( + auto writer, ManifestWriter::MakeWriter( + table_->metadata()->format_version, kInvalidSnapshotId, path, + file_io_, spec_, schema_, ManifestContent::kData, first_row_id)); + for (const auto& [file, snapshot_id] : files_to_write) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry( + file, snapshot_id, TableMetadata::kInitialSequenceNumber)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + return writer->ToManifestFile(); + } + + Result WriteAddedManifest( + const std::string& name, const std::vector>& files, + std::optional entry_snapshot_id = std::nullopt, + std::optional data_sequence_number = std::nullopt) { + auto path = std::format("{}/metadata/{}.avro", table_location_, name); + ICEBERG_ASSIGN_OR_RAISE( + auto writer, ManifestWriter::MakeWriter(table_->metadata()->format_version, + entry_snapshot_id, path, file_io_, spec_, + schema_, ManifestContent::kData)); + for (const auto& file : files) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteAddedEntry(file, data_sequence_number)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + ICEBERG_ASSIGN_OR_RAISE(auto manifest, writer->ToManifestFile()); + manifest.added_snapshot_id = kInvalidSnapshotId; + return manifest; + } + + Result WriteDeletedManifest( + const std::string& name, const std::vector>& files, + std::optional entry_snapshot_id = std::nullopt, + int64_t data_sequence_number = TableMetadata::kInitialSequenceNumber) { + auto path = std::format("{}/metadata/{}.avro", table_location_, name); + ICEBERG_ASSIGN_OR_RAISE( + auto writer, ManifestWriter::MakeWriter(table_->metadata()->format_version, + entry_snapshot_id, path, file_io_, spec_, + schema_, ManifestContent::kData)); + for (const auto& file : files) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteDeletedEntry(file, data_sequence_number)); + } + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + return writer->ToManifestFile(); + } + + std::shared_ptr spec_; + std::shared_ptr schema_; + std::shared_ptr file_a_; + std::shared_ptr file_b_; + std::shared_ptr file_c_; + std::shared_ptr file_d_; + std::shared_ptr<::testing::NiceMock> mock_catalog_; +}; + +TEST_P(RewriteManifestsTest, RewriteManifestsAppendedDirectly) { + SetSnapshotIdInheritanceEnabled(); + ICEBERG_UNWRAP_OR_FAIL(auto new_manifest, + WriteAddedManifest("manifest-file-1", {file_a_})); + + ICEBERG_UNWRAP_OR_FAIL(auto append, table_->NewFastAppend()); + append->AppendManifest(new_manifest); + EXPECT_THAT(append->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return ""; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {append_snapshot_id}); +} + +TEST_P(RewriteManifestsTest, RewriteManifestsWithScanExecutor) { + SetSnapshotIdInheritanceEnabled(); + ICEBERG_UNWRAP_OR_FAIL(auto new_manifest, + WriteAddedManifest("manifest-file-1", {file_a_})); + + ICEBERG_UNWRAP_OR_FAIL(auto append, table_->NewFastAppend()); + append->AppendManifest(new_manifest); + EXPECT_THAT(append->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + test::ThreadExecutor executor; + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return ""; }).ScanManifestsWith(executor); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + EXPECT_GT(executor.submit_count(), 0); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntries(manifests[0], {file_a_->file_path}, {ManifestStatus::kExisting}); +} + +TEST_P(RewriteManifestsTest, RewriteManifestsGeneratedAndAppendedDirectly) { + SetSnapshotIdInheritanceEnabled(); + ICEBERG_UNWRAP_OR_FAIL(auto new_manifest, + WriteAddedManifest("manifest-file-1", {file_a_})); + + ICEBERG_UNWRAP_OR_FAIL(auto append_manifest, table_->NewFastAppend()); + append_manifest->AppendManifest(new_manifest); + EXPECT_THAT(append_manifest->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifest_append_snapshot, table_->current_snapshot()); + const int64_t manifest_append_id = manifest_append_snapshot->snapshot_id; + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto file_append_snapshot, table_->current_snapshot()); + const int64_t file_append_id = file_append_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 2U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return ""; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, + {manifest_append_id, file_append_id}); +} + +TEST_P(RewriteManifestsTest, RewriteManifestsWithoutClusterBy) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_id = append_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + EXPECT_EQ(manifests[0].manifest_path, before[0].manifest_path); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kAdded}, {append_id}); +} + +TEST_P(RewriteManifestsTest, RewriteAfterV2ToV3UpgradeAssignsRowIds) { + if (format_version() != 2) { + GTEST_SKIP() << "This regression starts from a v2 table"; + } + + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto original_manifests, CurrentManifests()); + ASSERT_EQ(original_manifests.size(), 2U); + for (const auto& manifest : original_manifests) { + EXPECT_FALSE(manifest.first_row_id.has_value()); + } + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kFormatVersion.key()), "3"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + EXPECT_EQ(table_->metadata()->format_version, 3); + EXPECT_EQ(table_->metadata()->next_row_id, TableMetadata::kInitialRowId); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return ""; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + EXPECT_EQ(manifests[0].first_row_id, + std::make_optional(TableMetadata::kInitialRowId)); + + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifests[0])); + ASSERT_EQ(entries.size(), 2U); + + int64_t next_row_id = TableMetadata::kInitialRowId; + int64_t assigned_rows = 0; + for (const auto& entry : entries) { + ASSERT_NE(entry.data_file, nullptr); + ASSERT_TRUE(entry.data_file->first_row_id.has_value()); + EXPECT_EQ(entry.data_file->first_row_id.value(), next_row_id); + next_row_id += entry.data_file->record_count; + assigned_rows += entry.data_file->record_count; + } + EXPECT_EQ(table_->metadata()->next_row_id, + TableMetadata::kInitialRowId + assigned_rows); +} + +TEST_P(RewriteManifestsTest, ReplaceManifestsSeparate) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_id = append_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto manifest_a, + ManifestContainingPath(manifests, file_a_->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto manifest_b, + ManifestContainingPath(manifests, file_b_->file_path)); + ExpectManifestEntriesWithSnapshotIds(*manifest_a, {file_a_->file_path}, + {ManifestStatus::kExisting}, {append_id}); + ExpectManifestEntriesWithSnapshotIds(*manifest_b, {file_b_->file_path}, + {ManifestStatus::kExisting}, {append_id}); +} + +TEST_P(RewriteManifestsTest, ReplaceManifestsConsolidate) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_a, table_->current_snapshot()); + const int64_t append_id_a = append_snapshot_a->snapshot_id; + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_b, table_->current_snapshot()); + const int64_t append_id_b = append_snapshot_b->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 2U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, {append_id_a, append_id_b}); +} + +TEST_P(RewriteManifestsTest, ReplaceManifestsWithFilter) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_a, table_->current_snapshot()); + const int64_t append_id_a = append_snapshot_a->snapshot_id; + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_b, table_->current_snapshot()); + const int64_t append_id_b = append_snapshot_b->snapshot_id; + ASSERT_THAT(AppendFiles({file_c_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_c, table_->current_snapshot()); + const int64_t append_id_c = append_snapshot_c->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 3U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + rewrite->RewriteIf([&](const ManifestFile& manifest) { + auto entries_result = ReadManifestEntries(manifest); + if (!entries_result.has_value() || entries_result.value().empty()) { + return false; + } + return entries_result.value()[0].data_file->file_path != file_a_->file_path; + }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + + const ManifestFile* kept_manifest = nullptr; + const ManifestFile* rewritten_manifest = nullptr; + for (const auto& manifest : manifests) { + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + ASSERT_FALSE(entries.empty()); + ASSERT_NE(entries[0].data_file, nullptr); + if (entries[0].data_file->file_path == file_a_->file_path) { + kept_manifest = &manifest; + } else { + rewritten_manifest = &manifest; + } + } + + ASSERT_NE(rewritten_manifest, nullptr); + ExpectManifestEntriesWithSnapshotIds( + *rewritten_manifest, {file_b_->file_path, file_c_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, {append_id_b, append_id_c}); + + ASSERT_NE(kept_manifest, nullptr); + ExpectManifestEntriesWithSnapshotIds(*kept_manifest, {file_a_->file_path}, + {ManifestStatus::kAdded}, {append_id_a}); +} + +TEST_P(RewriteManifestsTest, ReplaceManifestsMaxSize) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kManifestTargetSizeBytes.key()), "1"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + std::ranges::sort(manifests, {}, &ManifestFile::manifest_path); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {append_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, + {ManifestStatus::kExisting}, {append_snapshot_id}); +} + +TEST_P(RewriteManifestsTest, CleansRolledRewriteManifestsAfterApplyFailure) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kManifestTargetSizeBytes.key()), "1"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto failure_state = std::make_shared(); + failure_state->fail_on_manifest_output = 2; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", failure_state)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Injected rewrite manifest writer failure")); + + ASSERT_GE(failure_state->manifest_outputs.size(), 2U); + const auto& closed_manifest_path = failure_state->manifest_outputs[0]; + EXPECT_THAT(failure_state->deleted_files, ::testing::Contains(closed_manifest_path)); + EXPECT_FALSE(FileExists(closed_manifest_path)); +} + +TEST_P(RewriteManifestsTest, ClosesOpenRewriteWritersAfterApplyFailure) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + + auto failure_state = std::make_shared(); + failure_state->fail_on_manifest_output = 3; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", failure_state)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Injected rewrite manifest writer failure")); + + ASSERT_EQ(failure_state->manifest_outputs.size(), 3U); + EXPECT_THAT(failure_state->manifest_close_attempts, + ::testing::UnorderedElementsAre(failure_state->manifest_outputs[0], + failure_state->manifest_outputs[1])); + for (size_t i = 0; i < 2; ++i) { + EXPECT_THAT(failure_state->deleted_files, + ::testing::Contains(failure_state->manifest_outputs[i])); + EXPECT_FALSE(FileExists(failure_state->manifest_outputs[i])); + } +} + +TEST_P(RewriteManifestsTest, ClosesAllRewriteWritersAfterCloseFailure) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + + auto failure_state = std::make_shared(); + failure_state->fail_on_manifest_close = 1; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", failure_state)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Injected rewrite manifest close failure")); + + ASSERT_EQ(failure_state->manifest_outputs.size(), 3U); + ASSERT_EQ(failure_state->manifest_close_attempts.size(), 3U); + EXPECT_THAT(failure_state->manifest_close_attempts, + ::testing::UnorderedElementsAre(failure_state->manifest_outputs[0], + failure_state->manifest_outputs[1], + failure_state->manifest_outputs[2])); + for (size_t i = 0; i < failure_state->manifest_close_attempts.size(); ++i) { + const auto& closed_manifest_path = failure_state->manifest_close_attempts[i]; + EXPECT_THAT(failure_state->deleted_files, ::testing::Contains(closed_manifest_path)); + EXPECT_FALSE(FileExists(closed_manifest_path)); + } +} + +TEST_P(RewriteManifestsTest, PreservesRewriteFailureWhenClosingWritersFails) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + + auto failure_state = std::make_shared(); + failure_state->fail_on_manifest_output = 3; + failure_state->fail_on_manifest_close = 1; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", failure_state)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Injected rewrite manifest writer failure")); + EXPECT_THAT(result, HasErrorMessage("additionally failed to close manifest writer")); + EXPECT_THAT(result, HasErrorMessage("Injected rewrite manifest close failure")); + + ASSERT_EQ(failure_state->manifest_outputs.size(), 3U); + ASSERT_EQ(failure_state->manifest_close_attempts.size(), 2U); + for (const auto& manifest_path : failure_state->manifest_outputs) { + EXPECT_THAT(failure_state->deleted_files, ::testing::Contains(manifest_path)); + EXPECT_FALSE(FileExists(manifest_path)); + } +} + +TEST_P(RewriteManifestsTest, ClosesWritersWhenClusterByThrows) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + + for (bool explicit_transaction : {false, true}) { + for (int close_failure : {0, 1, 2}) { + SCOPED_TRACE(std::format("explicit_transaction={}, close_failure={}", + explicit_transaction, close_failure)); + auto state = std::make_shared(); + state->fail_on_manifest_close = close_failure == 1 ? 1 : 0; + state->throw_on_manifest_close = close_failure == 2 ? 1 : 0; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", state)); + + std::shared_ptr txn; + std::shared_ptr rewrite; + if (explicit_transaction) { + ICEBERG_UNWRAP_OR_FAIL(txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(rewrite, txn->NewRewriteManifests()); + } else { + ICEBERG_UNWRAP_OR_FAIL(rewrite, table_->NewRewriteManifests()); + } + rewrite->ClusterBy([calls = 0](const DataFile& file) mutable -> std::string { + if (++calls == 3) { + throw std::runtime_error("Injected cluster failure"); + } + return file.file_path; + }); + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Injected cluster failure")); + if (txn) { + EXPECT_EQ(txn->state(), TransactionState::kFailed); + } + ASSERT_EQ(state->manifest_outputs.size(), 2U); + EXPECT_THAT(state->manifest_close_attempts, + ::testing::UnorderedElementsAreArray(state->manifest_outputs)); + EXPECT_TRUE(state->open_manifest_paths.empty()); + EXPECT_TRUE(state->deleted_open_files.empty()); + for (const auto& path : state->manifest_outputs) { + EXPECT_THAT(state->deleted_files, ::testing::Contains(path)); + EXPECT_FALSE(FileExists(path)); + } + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + EXPECT_EQ(after, before); + } + } +} + +TEST_P(RewriteManifestsTest, CleansWritersAfterTransactionApplyFailure) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + auto state = std::make_shared(); + state->fail_on_manifest_output = 3; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", state)); + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, txn->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + EXPECT_THAT(rewrite->Commit(), IsError(ErrorKind::kIOError)); + EXPECT_EQ(txn->state(), TransactionState::kFailed); + ASSERT_EQ(state->manifest_outputs.size(), 3U); + EXPECT_THAT(state->manifest_close_attempts, + ::testing::UnorderedElementsAre(state->manifest_outputs[0], + state->manifest_outputs[1])); + EXPECT_TRUE(state->open_manifest_paths.empty()); + EXPECT_TRUE(state->deleted_open_files.empty()); + for (const auto& path : state->manifest_outputs) { + EXPECT_THAT(state->deleted_files, ::testing::Contains(path)); + EXPECT_FALSE(FileExists(path)); + } + EXPECT_THAT(txn->Commit(), IsError(ErrorKind::kValidationFailed)); + ASSERT_THAT(txn->Abort(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + EXPECT_EQ(after, before); +} + +TEST_P(RewriteManifestsTest, ClosesAndCleansCopiedManifestOnFailure) { + if (format_version() != 1) { + GTEST_SKIP() << "Manifest copying is only required without snapshot ID inheritance"; + } + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + ICEBERG_UNWRAP_OR_FAIL( + auto invalid_manifest, + WriteAddedManifest("copy-invalid-entry", {file_a_}, snapshot->snapshot_id)); + // Counts pass AddManifest validation; reading the entries must still reject it. + invalid_manifest.added_files_count = 0; + invalid_manifest.existing_files_count = 1; + ICEBERG_UNWRAP_OR_FAIL( + auto valid_manifest, + WriteExistingManifest("copy-close-failure", snapshot->snapshot_id, {file_a_})); + + for (bool explicit_transaction : {false, true}) { + for (bool invalid_entry : {false, true}) { + SCOPED_TRACE(std::format("explicit_transaction={}, invalid_entry={}", + explicit_transaction, invalid_entry)); + auto state = std::make_shared(); + state->fail_on_manifest_close = invalid_entry ? 0 : 1; + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", state)); + std::shared_ptr txn; + std::shared_ptr rewrite; + if (explicit_transaction) { + ICEBERG_UNWRAP_OR_FAIL(txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(rewrite, txn->NewRewriteManifests()); + } else { + ICEBERG_UNWRAP_OR_FAIL(rewrite, table_->NewRewriteManifests()); + } + const auto& source = invalid_entry ? invalid_manifest : valid_manifest; + rewrite->DeleteManifest(before[0]).AddManifest(source); + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(invalid_entry ? ErrorKind::kValidationFailed + : ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage( + invalid_entry ? "Cannot add manifest with added files" + : "Injected rewrite manifest close failure")); + if (txn) { + EXPECT_EQ(txn->state(), TransactionState::kFailed); + } + ASSERT_EQ(state->manifest_outputs.size(), 1U); + EXPECT_THAT(state->manifest_close_attempts, + ::testing::ElementsAre(state->manifest_outputs[0])); + EXPECT_TRUE(state->open_manifest_paths.empty()); + EXPECT_TRUE(state->deleted_open_files.empty()); + EXPECT_THAT(state->deleted_files, ::testing::Contains(state->manifest_outputs[0])); + EXPECT_FALSE(FileExists(state->manifest_outputs[0])); + EXPECT_TRUE(FileExists(source.manifest_path)); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + EXPECT_EQ(after, before); + } + } +} + +TEST_P(RewriteManifestsTest, ConcurrentRewriteManifest) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_a, table_->current_snapshot()); + const int64_t append_id_a = append_snapshot_a->snapshot_id; + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_b, table_->current_snapshot()); + const int64_t append_id_b = append_snapshot_b->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 2U); + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, txn->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto concurrent, table_->NewRewriteManifests()); + concurrent->ClusterBy([](const DataFile&) { return "file"; }); + concurrent->RewriteIf([&](const ManifestFile& manifest) { + auto entries_result = ReadManifestEntries(manifest); + if (!entries_result.has_value() || entries_result.value().empty()) { + return false; + } + return entries_result.value()[0].data_file->file_path != file_a_->file_path; + }); + EXPECT_THAT(concurrent->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto after_concurrent, CurrentManifests()); + ASSERT_EQ(after_concurrent.size(), 2U); + + ASSERT_THAT(txn->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, {append_id_a, append_id_b}); +} + +TEST_P(RewriteManifestsTest, AppendDuringRewriteManifest) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_a, table_->current_snapshot()); + const int64_t append_id_a = append_snapshot_a->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, txn->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot_b, table_->current_snapshot()); + const int64_t append_id_b = append_snapshot_b->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto after_append, CurrentManifests()); + ASSERT_EQ(after_append.size(), 2U); + + // commit the rewrite manifests in progress + ASSERT_THAT(txn->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + + // Retry cleanup discards the staged rewrite and reclusters the latest manifests. + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, {append_id_a, append_id_b}); +} + +TEST_P(RewriteManifestsTest, RewriteManifestDuringAppend) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto append, txn->NewFastAppend()); + append->AppendFile(file_b_); + ASSERT_THAT(append->Commit(), IsOk()); + + // rewrite the manifests - only affects the first + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto rewritten_manifests, CurrentManifests()); + ASSERT_EQ(rewritten_manifests.size(), 1U); + + // commit the append in progress + ASSERT_THAT(txn->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto file_append_snapshot, table_->current_snapshot()); + const int64_t file_append_id = file_append_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_b_->file_path}, + {ManifestStatus::kAdded}, {file_append_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_a_->file_path}, + {ManifestStatus::kExisting}, {append_snapshot_id}); +} + +TEST_P(RewriteManifestsTest, BasicManifestReplacement) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ASSERT_THAT(AppendFiles({file_c_, file_d_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_a, WriteExistingManifest("rewrite-a", first_snapshot_id, {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_b, WriteExistingManifest("rewrite-b", first_snapshot_id, {file_b_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(manifest_a) + .AddManifest(manifest_b); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 3U); + if (GetParam() == 1) { + EXPECT_NE(manifests[0].manifest_path, manifest_a.manifest_path); + EXPECT_NE(manifests[1].manifest_path, manifest_b.manifest_path); + } else { + EXPECT_EQ(manifests[0].manifest_path, manifest_a.manifest_path); + EXPECT_EQ(manifests[1].manifest_path, manifest_b.manifest_path); + } + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[2], + {file_c_->file_path, file_d_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {second_snapshot_id, second_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto after, table_->current_snapshot()); + ValidateSummary(after, /*replaced=*/1, /*kept=*/1, /*created=*/2, + /*entry_count=*/0); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementWithUnassignedSnapshotId) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto original_snapshot, table_->current_snapshot()); + + for (bool inherit_snapshot_id : {false, true}) { + SCOPED_TRACE(inherit_snapshot_id); + if (inherit_snapshot_id) { + SetSnapshotIdInheritanceEnabled(); + } + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(before[0])); + auto path = std::format("{}/metadata/unassigned-{}.avro", table_location_, + inherit_snapshot_id); + ICEBERG_UNWRAP_OR_FAIL( + auto writer, ManifestWriter::MakeWriter( + format_version(), std::nullopt, path, file_io_, spec_, schema_, + ManifestContent::kData, before[0].first_row_id)); + for (const auto& entry : entries) { + ASSERT_THAT(writer->WriteExistingEntry(entry), IsOk()); + } + ASSERT_THAT(writer->Close(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto replacement, writer->ToManifestFile()); + ASSERT_FALSE(replacement.added_snapshot_id.has_value()); + + auto assigned_manifest = replacement; + assigned_manifest.added_snapshot_id = original_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto invalid_rewrite, table_->NewRewriteManifests()); + invalid_rewrite->DeleteManifest(before[0]).AddManifest(assigned_manifest); + EXPECT_THAT(invalid_rewrite->Commit(), + HasErrorMessage("Snapshot id must be assigned during commit")); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(before[0]).AddManifest(replacement); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_NE(snapshot->snapshot_id, original_snapshot->snapshot_id); + EXPECT_EQ(manifests[0].added_snapshot_id, snapshot->snapshot_id); + if (inherit_snapshot_id || format_version() > 1) { + EXPECT_EQ(manifests[0].manifest_path, replacement.manifest_path); + } else { + EXPECT_NE(manifests[0].manifest_path, replacement.manifest_path); + } + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, + {original_snapshot->snapshot_id, original_snapshot->snapshot_id}); + } +} + +TEST_P(RewriteManifestsTest, BasicManifestReplacementWithSnapshotIdInheritance) { + SetSnapshotIdInheritanceEnabled(); + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ASSERT_THAT(AppendFiles({file_c_, file_d_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_a, + WriteExistingManifest("rewrite-inherit-a", first_snapshot_id, {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_b, + WriteExistingManifest("rewrite-inherit-b", first_snapshot_id, {file_b_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(manifest_a) + .AddManifest(manifest_b); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 3U); + EXPECT_EQ(manifests[0].manifest_path, manifest_a.manifest_path); + EXPECT_EQ(manifests[1].manifest_path, manifest_b.manifest_path); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[2], + {file_c_->file_path, file_d_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {second_snapshot_id, second_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto after, table_->current_snapshot()); + ValidateSummary(after, /*replaced=*/1, /*kept=*/1, /*created=*/2, + /*entry_count=*/0); + + // validate that any subsequent operation does not fail + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFromRowFilter(Expressions::AlwaysTrue()); + EXPECT_THAT(delete_files->Commit(), IsOk()); +} + +TEST_P(RewriteManifestsTest, WithMultiplePartitionSpec) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto initial_manifests, CurrentManifests()); + ASSERT_EQ(initial_manifests.size(), 1U); + const int32_t initial_spec_id = initial_manifests[0].partition_spec_id; + ICEBERG_UNWRAP_OR_FAIL(auto initial_spec, + table_->metadata()->PartitionSpecById(initial_spec_id)); + ASSERT_EQ(initial_spec->fields().size(), 1U); + const int32_t initial_partition_field_id = initial_spec->fields()[0].field_id(); + + // Build the new spec using the table's schema, which uses fresh partition IDs. + ICEBERG_UNWRAP_OR_FAIL(auto update_spec, table_->NewUpdatePartitionSpec()); + update_spec->AddField(Expressions::Bucket("y", 16)) + .AddField(Expressions::Bucket("z", 4)); + EXPECT_THAT(update_spec->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto new_spec, table_->spec()); + ASSERT_EQ(new_spec->fields().size(), 3U); + EXPECT_EQ(new_spec->fields()[1].name(), "y_bucket_16"); + EXPECT_EQ(new_spec->fields()[1].transform()->ToString(), "bucket[16]"); + EXPECT_GT(new_spec->fields()[1].field_id(), initial_partition_field_id); + EXPECT_EQ(new_spec->fields()[2].name(), "z_bucket_4"); + EXPECT_EQ(new_spec->fields()[2].transform()->ToString(), "bucket[4]"); + EXPECT_GT(new_spec->fields()[2].field_id(), new_spec->fields()[1].field_id()); + + std::vector partition_y{Literal::Long(/*x=*/5L), + Literal::Int(/*y_bucket_16=*/2), + Literal::Int(/*z_bucket_4=*/3)}; + auto file_y = MakeDataFile("y", new_spec, /*record_count=*/1, /*size=*/10, + std::move(partition_y)); + std::vector partition_z{Literal::Long(/*x=*/7L), + Literal::Int(/*y_bucket_16=*/2), + Literal::Int(/*z_bucket_4=*/4)}; + auto file_z = MakeDataFile("z", new_spec, /*record_count=*/1, /*size=*/10, + std::move(partition_z)); + ASSERT_THAT(AppendFiles({file_y}), IsOk()); + ASSERT_THAT(AppendFiles({file_z}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 3U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + // try to cluster in 1 manifest file, but because of 2 partition specs + // we should still have 2 manifest files. + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + EXPECT_NE(manifests[1].partition_spec_id, manifests[0].partition_spec_id); + MatchNumberOfManifestFileWithSpecId(manifests, initial_spec_id, + /*expected_manifest_count=*/1); + MatchNumberOfManifestFileWithSpecId(manifests, new_spec->spec_id(), + /*expected_manifest_count=*/1); + + EXPECT_EQ(manifests[0].existing_files_count.value_or(-1), 2); + EXPECT_EQ(manifests[1].existing_files_count.value_or(-1), 2); +} + +TEST_P(RewriteManifestsTest, ManifestSizeWithMultiplePartitionSpec) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto initial_manifests, CurrentManifests()); + ASSERT_EQ(initial_manifests.size(), 1U); + const int32_t initial_spec_id = initial_manifests[0].partition_spec_id; + + // Build the new spec using the table's schema, which uses fresh partition IDs. + ICEBERG_UNWRAP_OR_FAIL(auto update_spec, table_->NewUpdatePartitionSpec()); + update_spec->AddField(Expressions::Bucket("y", 16)) + .AddField(Expressions::Bucket("z", 4)); + EXPECT_THAT(update_spec->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto new_spec, table_->spec()); + + std::vector partition_y{Literal::Long(/*x=*/5L), + Literal::Int(/*y_bucket_16=*/2), + Literal::Int(/*z_bucket_4=*/3)}; + auto file_y = MakeDataFile("y", new_spec, /*record_count=*/1, /*size=*/10, + std::move(partition_y)); + ASSERT_THAT(AppendFiles({file_y}), IsOk()); + + std::vector partition_z{Literal::Long(/*x=*/7L), + Literal::Int(/*y_bucket_16=*/2), + Literal::Int(/*z_bucket_4=*/4)}; + auto file_z = MakeDataFile("z", new_spec, /*record_count=*/1, /*size=*/10, + std::move(partition_z)); + ASSERT_THAT(AppendFiles({file_z}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 3U); + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kManifestTargetSizeBytes.key()), "1"); + EXPECT_THAT(props->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "file"; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 4U); + + MatchNumberOfManifestFileWithSpecId(manifests, initial_spec_id, + /*expected_manifest_count=*/2); + MatchNumberOfManifestFileWithSpecId(manifests, new_spec->spec_id(), + /*expected_manifest_count=*/2); + + EXPECT_EQ(manifests[0].existing_files_count.value_or(-1), 1); + EXPECT_EQ(manifests[1].existing_files_count.value_or(-1), 1); + EXPECT_EQ(manifests[2].existing_files_count.value_or(-1), 1); + EXPECT_EQ(manifests[3].existing_files_count.value_or(-1), 1); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementConcurrentAppend) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_a, + WriteExistingManifest("concurrent-append-a", first_snapshot_id, {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_b, + WriteExistingManifest("concurrent-append-b", first_snapshot_id, {file_b_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(manifest_a) + .AddManifest(manifest_b); + + ASSERT_THAT(AppendFiles({file_c_, file_d_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto after_append, CurrentManifests()); + ASSERT_EQ(after_append.size(), 2U); + + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 3U); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[2], + {file_c_->file_path, file_d_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {second_snapshot_id, second_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto after, table_->current_snapshot()); + ValidateSummary(after, /*replaced=*/1, /*kept=*/1, /*created=*/2, + /*entry_count=*/0); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementConcurrentDelete) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ASSERT_THAT(AppendFiles({file_c_, file_d_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_a, + WriteExistingManifest("concurrent-delete-a", first_snapshot_id, {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_b, + WriteExistingManifest("concurrent-delete-b", first_snapshot_id, {file_b_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(manifest_a) + .AddManifest(manifest_b); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_c_); + EXPECT_THAT(delete_files->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto third_snapshot, table_->current_snapshot()); + const int64_t third_snapshot_id = third_snapshot->snapshot_id; + + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 3U); + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + ExpectManifestEntriesWithSnapshotIds( + manifests[2], {file_c_->file_path, file_d_->file_path}, + {ManifestStatus::kDeleted, ManifestStatus::kExisting}, + {third_snapshot_id, second_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto after, table_->current_snapshot()); + ValidateSummary(after, /*replaced=*/1, /*kept=*/1, /*created=*/2, + /*entry_count=*/0); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementConcurrentConflictingDelete) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_a, + WriteExistingManifest("conflicting-delete-a", first_snapshot_id, {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto manifest_b, + WriteExistingManifest("conflicting-delete-b", first_snapshot_id, {file_b_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(manifest_a) + .AddManifest(manifest_b); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_a_); + EXPECT_THAT(delete_files->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("could not be found in the latest snapshot")); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementCombinedWithRewrite) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_manifests, CurrentManifests()); + ASSERT_EQ(first_manifests.size(), 1U); + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + ASSERT_THAT(AppendFiles({file_c_}), IsOk()); + ASSERT_THAT(AppendFiles({file_d_}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL( + auto new_manifest, + WriteExistingManifest("combined-a", first_snapshot_id, {file_a_})); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_manifests[0]) + .AddManifest(new_manifest) + .ClusterBy([](const DataFile&) { return "const-value"; }) + .RewriteIf([&](const ManifestFile& manifest) { + auto entries_result = ReadManifestEntries(manifest); + if (!entries_result.has_value() || entries_result.value().empty()) { + return false; + } + return entries_result.value()[0].data_file->file_path != file_b_->file_path; + }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 3U); + + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + + ExpectManifestEntriesWithSnapshotIds(manifests[2], {file_b_->file_path}, + {ManifestStatus::kAdded}, {second_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ValidateSummary(snapshot, /*replaced=*/3, /*kept=*/1, /*created=*/2, + /*entry_count=*/2); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementCombinedWithRewriteConcurrentDelete) { + SetManifestMergeEnabled(false); + + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + + ASSERT_THAT(AppendFiles({file_c_}), IsOk()); + ASSERT_EQ(table_->snapshots().size(), 3U); + + ICEBERG_UNWRAP_OR_FAIL( + auto new_manifest, + WriteExistingManifest("combined-concurrent-delete-a", first_snapshot_id, {file_a_}, + ManifestContent::kData, nullptr, + first_snapshot->sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, txn->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifests[0]) + .AddManifest(new_manifest) + .ClusterBy([](const DataFile&) { return "const-value"; }); + + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_c_); + EXPECT_THAT(delete_files->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + ASSERT_THAT(txn->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_b_->file_path}, + {ManifestStatus::kExisting}, {second_snapshot_id}); + + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_a_->file_path}, + {ManifestStatus::kExisting}, {first_snapshot_id}); + + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ValidateSummary(snapshot, /*replaced=*/3, /*kept=*/0, /*created=*/2, + /*entry_count=*/1); +} + +TEST_P(RewriteManifestsTest, InvalidUsage) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + const auto manifest = manifests[0]; + + ICEBERG_UNWRAP_OR_FAIL( + auto invalid_added_manifest, + WriteAddedManifest("manifest-file-2", {file_a_}, snapshot->snapshot_id, + snapshot->sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite_added, table_->NewRewriteManifests()); + rewrite_added->DeleteManifest(manifest).AddManifest(invalid_added_manifest); + auto added_result = rewrite_added->Commit(); + EXPECT_THAT(added_result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(added_result, HasErrorMessage("Cannot add manifest with added files")); + + ICEBERG_UNWRAP_OR_FAIL( + auto invalid_deleted_manifest, + WriteDeletedManifest("manifest-file-3", {file_a_}, snapshot->snapshot_id, + snapshot->sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite_deleted, table_->NewRewriteManifests()); + rewrite_deleted->DeleteManifest(manifest).AddManifest(invalid_deleted_manifest); + auto deleted_result = rewrite_deleted->Commit(); + EXPECT_THAT(deleted_result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(deleted_result, HasErrorMessage("Cannot add manifest with deleted files")); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite_missing, table_->NewRewriteManifests()); + rewrite_missing->DeleteManifest(manifest); + auto missing_result = rewrite_missing->Commit(); + EXPECT_THAT(missing_result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(missing_result, + HasErrorMessage("Replaced and created manifests must have the same")); +} + +TEST_P(RewriteManifestsTest, RebuildsRewriteAndReplacementAfterCommitConflict) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto first_manifests, CurrentManifests()); + ASSERT_EQ(first_manifests.size(), 1U); + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL( + auto replacement, + WriteExistingManifest("retry-replacement", first_snapshot->snapshot_id, {file_a_})); + + SetCommitRetryProperties(1); + BindTableWithFailingCommits(/*failures=*/1); + + std::unordered_set deleted_paths; + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_manifests[0]) + .AddManifest(replacement) + .ClusterBy([](const DataFile&) { return "file"; }) + .Set("retry-property", "retained") + .DeleteWith([&](const std::string& path) { + deleted_paths.insert(path); + return file_io_->DeleteFile(path); + }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + EXPECT_FALSE(deleted_paths.empty()); + EXPECT_FALSE(deleted_paths.contains(replacement.manifest_path)); + EXPECT_TRUE(FileExists(replacement.manifest_path)); + for (const auto& path : deleted_paths) { + EXPECT_FALSE(FileExists(path)); + } + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 2U); + for (const auto& manifest : manifests) { + EXPECT_FALSE(deleted_paths.contains(manifest.manifest_path)); + EXPECT_TRUE(FileExists(manifest.manifest_path)); + } + ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_b_->file_path}, + {ManifestStatus::kExisting}, + {second_snapshot->snapshot_id}); + ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_a_->file_path}, + {ManifestStatus::kExisting}, + {first_snapshot->snapshot_id}); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ValidateSummary(snapshot, /*replaced=*/2, /*kept=*/0, /*created=*/2, + /*entry_count=*/1); + EXPECT_THAT(snapshot->summary, + ::testing::Contains(::testing::Pair("retry-property", "retained"))); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementFailure) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + const auto first_snapshot_manifest = first_snapshot_manifests[0]; + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(second_snapshot_manifests.size(), 2U); + const auto second_snapshot_manifest = second_snapshot_manifests[0]; + + ICEBERG_UNWRAP_OR_FAIL( + auto new_manifest, + WriteExistingManifest("manifest-file", {{file_a_, first_snapshot_id}, + {file_b_, second_snapshot_id}})); + EXPECT_TRUE(FileExists(new_manifest.manifest_path)); + + SetCommitRetryProperties(1); + BindTableWithFailingCommits(/*failures=*/5); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifest) + .DeleteManifest(second_snapshot_manifest) + .AddManifest(new_manifest); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(result, HasErrorMessage("Injected failure")); + EXPECT_TRUE(FileExists(new_manifest.manifest_path)); +} + +TEST_P(RewriteManifestsTest, ManifestReplacementFailureWithSnapshotIdInheritance) { + SetSnapshotIdInheritanceEnabled(); + + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + const int64_t first_snapshot_id = first_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(first_snapshot_manifests.size(), 1U); + const auto first_snapshot_manifest = first_snapshot_manifests[0]; + + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot, table_->current_snapshot()); + const int64_t second_snapshot_id = second_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto second_snapshot_manifests, CurrentManifests()); + ASSERT_EQ(second_snapshot_manifests.size(), 2U); + const auto second_snapshot_manifest = second_snapshot_manifests[0]; + + ICEBERG_UNWRAP_OR_FAIL( + auto new_manifest, + WriteExistingManifest("manifest-file", {{file_a_, first_snapshot_id}, + {file_b_, second_snapshot_id}})); + EXPECT_TRUE(FileExists(new_manifest.manifest_path)); + + SetCommitRetryProperties(1); + BindTableWithFailingCommits(/*failures=*/5); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(first_snapshot_manifest) + .DeleteManifest(second_snapshot_manifest) + .AddManifest(new_manifest); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(result, HasErrorMessage("Injected failure")); + EXPECT_TRUE(FileExists(new_manifest.manifest_path)); +} + +TEST_P(RewriteManifestsTest, RewriteManifestsOnBranchUnsupported) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ToBranch("someBranch"); + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, + HasErrorMessage("Cannot commit to branch someBranch: RewriteManifests " + "does not support branch commits")); +} + +TEST_P(RewriteManifestsTest, RewriteDataManifestsPreservesDeletes) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + const int64_t append_snapshot_sequence_number = append_snapshot->sequence_number; + + auto delete_a = MakeDeleteFile("a-pos-deletes", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("a2-deletes", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto delete_snapshot, table_->current_snapshot()); + const int64_t delete_snapshot_id = delete_snapshot->snapshot_id; + const int64_t delete_snapshot_sequence_number = delete_snapshot->sequence_number; + + ICEBERG_UNWRAP_OR_FAIL(auto data_before, CurrentManifests()); + ICEBERG_UNWRAP_OR_FAIL(auto deletes_before, CurrentDeleteManifests()); + ASSERT_EQ(data_before.size(), 1U); + ASSERT_EQ(deletes_before.size(), 1U); + const std::string delete_manifest_path = deletes_before[0].manifest_path; + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, CurrentManifests()); + ASSERT_EQ(data_manifests.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto data_manifest_a, + ManifestContainingPath(data_manifests, file_a_->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto data_manifest_b, + ManifestContainingPath(data_manifests, file_b_->file_path)); + ExpectManifestEntriesWithSequenceNumbers( + *data_manifest_a, {file_a_->file_path}, {ManifestStatus::kExisting}, + {append_snapshot_id}, {append_snapshot_sequence_number}, + {append_snapshot_sequence_number}); + ExpectManifestEntriesWithSequenceNumbers( + *data_manifest_b, {file_b_->file_path}, {ManifestStatus::kExisting}, + {append_snapshot_id}, {append_snapshot_sequence_number}, + {append_snapshot_sequence_number}); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(delete_manifests.size(), 1U); + EXPECT_EQ(delete_manifests[0].manifest_path, delete_manifest_path); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[0], {delete_a->file_path, delete_a2->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {delete_snapshot_id, delete_snapshot_id}, + {delete_snapshot_sequence_number, delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number, delete_snapshot_sequence_number}); + ICEBERG_UNWRAP_OR_FAIL(auto delete_entries, ReadManifestEntries(delete_manifests[0])); + std::unordered_map delete_content_by_path; + for (const auto& entry : delete_entries) { + ASSERT_NE(entry.data_file, nullptr); + delete_content_by_path.emplace(entry.data_file->file_path, entry.data_file->content); + } + EXPECT_EQ(delete_content_by_path.at(delete_a->file_path), + DataFile::Content::kPositionDeletes); + EXPECT_EQ(delete_content_by_path.at(delete_a2->file_path), + DataFile::Content::kEqualityDeletes); +} + +TEST_P(RewriteManifestsTest, ReplaceDeleteManifestsOnly) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + const int64_t append_snapshot_sequence_number = append_snapshot->sequence_number; + + auto delete_a = MakeDeleteFile("replace-delete-a", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("replace-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto delete_snapshot, table_->current_snapshot()); + const int64_t delete_snapshot_id = delete_snapshot->snapshot_id; + const int64_t delete_snapshot_sequence_number = delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto original_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(original_delete_manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a, + WriteExistingManifest("delete-manifest-file-1", delete_snapshot_id, {delete_a}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a2, + WriteExistingManifest("delete-manifest-file-2", delete_snapshot_id, {delete_a2}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(original_delete_manifests[0]) + .AddManifest(new_delete_manifest_a) + .AddManifest(new_delete_manifest_a2); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, CurrentManifests()); + ASSERT_EQ(data_manifests.size(), 1U); + ExpectManifestEntriesWithSequenceNumbers( + data_manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {append_snapshot_id, append_snapshot_id}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(delete_manifests.size(), 2U); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[0], {delete_a->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[1], {delete_a2->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ICEBERG_UNWRAP_OR_FAIL(auto delete_entries_a, ReadManifestEntries(delete_manifests[0])); + ASSERT_EQ(delete_entries_a.size(), 1U); + ASSERT_NE(delete_entries_a[0].data_file, nullptr); + EXPECT_EQ(delete_entries_a[0].data_file->content, DataFile::Content::kPositionDeletes); + ICEBERG_UNWRAP_OR_FAIL(auto delete_entries_a2, + ReadManifestEntries(delete_manifests[1])); + ASSERT_EQ(delete_entries_a2.size(), 1U); + ASSERT_NE(delete_entries_a2[0].data_file, nullptr); + EXPECT_EQ(delete_entries_a2[0].data_file->content, DataFile::Content::kEqualityDeletes); +} + +TEST_P(RewriteManifestsTest, ReplaceDataAndDeleteManifests) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + const int64_t append_snapshot_sequence_number = append_snapshot->sequence_number; + + auto delete_a = MakeDeleteFile("replace-data-delete-a", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("replace-data-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto delete_snapshot, table_->current_snapshot()); + const int64_t delete_snapshot_id = delete_snapshot->snapshot_id; + const int64_t delete_snapshot_sequence_number = delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto original_data_manifests, CurrentManifests()); + ICEBERG_UNWRAP_OR_FAIL(auto original_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(original_data_manifests.size(), 1U); + ASSERT_EQ(original_delete_manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL(auto new_data_manifest_a, + WriteExistingManifest("manifest-file-1", append_snapshot_id, + {file_a_}, ManifestContent::kData, nullptr, + append_snapshot_sequence_number, + append_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL(auto new_data_manifest_b, + WriteExistingManifest("manifest-file-2", append_snapshot_id, + {file_b_}, ManifestContent::kData, nullptr, + append_snapshot_sequence_number, + append_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a, + WriteExistingManifest("delete-manifest-file-1", delete_snapshot_id, {delete_a}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a2, + WriteExistingManifest("delete-manifest-file-2", delete_snapshot_id, {delete_a2}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(original_data_manifests[0]) + .AddManifest(new_data_manifest_a) + .AddManifest(new_data_manifest_b) + .DeleteManifest(original_delete_manifests[0]) + .AddManifest(new_delete_manifest_a) + .AddManifest(new_delete_manifest_a2); + EXPECT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, CurrentManifests()); + ASSERT_EQ(data_manifests.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto data_manifest_a, + ManifestContainingPath(data_manifests, file_a_->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto data_manifest_b, + ManifestContainingPath(data_manifests, file_b_->file_path)); + ExpectManifestEntriesWithSequenceNumbers( + *data_manifest_a, {file_a_->file_path}, {ManifestStatus::kExisting}, + {append_snapshot_id}, {append_snapshot_sequence_number}, + {append_snapshot_sequence_number}); + ExpectManifestEntriesWithSequenceNumbers( + *data_manifest_b, {file_b_->file_path}, {ManifestStatus::kExisting}, + {append_snapshot_id}, {append_snapshot_sequence_number}, + {append_snapshot_sequence_number}); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(delete_manifests.size(), 2U); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[0], {delete_a->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ExpectManifestEntryContents(delete_manifests[0], {delete_a->file_path}, + {DataFile::Content::kPositionDeletes}); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[1], {delete_a2->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ExpectManifestEntryContents(delete_manifests[1], {delete_a2->file_path}, + {DataFile::Content::kEqualityDeletes}); +} + +TEST_P(RewriteManifestsTest, DeleteManifestReplacementConcurrentAppend) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + const int64_t append_snapshot_sequence_number = append_snapshot->sequence_number; + + auto delete_a = MakeDeleteFile("concurrent-append-delete-a", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("concurrent-append-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto delete_snapshot, table_->current_snapshot()); + const int64_t delete_snapshot_id = delete_snapshot->snapshot_id; + const int64_t delete_snapshot_sequence_number = delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto original_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(original_delete_manifests.size(), 1U); + + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a, + WriteExistingManifest("delete-manifest-file-1", delete_snapshot_id, {delete_a}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a2, + WriteExistingManifest("delete-manifest-file-2", delete_snapshot_id, {delete_a2}, + ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(original_delete_manifests[0]) + .AddManifest(new_delete_manifest_a) + .AddManifest(new_delete_manifest_a2); + + ASSERT_THAT(AppendFiles({file_c_, file_d_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto concurrent_snapshot, table_->current_snapshot()); + const int64_t concurrent_snapshot_id = concurrent_snapshot->snapshot_id; + const int64_t concurrent_snapshot_sequence_number = + concurrent_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto data_before_rewrite, CurrentManifests()); + ICEBERG_UNWRAP_OR_FAIL(auto deletes_before_rewrite, CurrentDeleteManifests()); + ASSERT_EQ(data_before_rewrite.size(), 2U); + ASSERT_EQ(deletes_before_rewrite.size(), 1U); + + EXPECT_THAT(rewrite->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite_snapshot, table_->current_snapshot()); + ValidateSummary(rewrite_snapshot, /*replaced=*/1, /*kept=*/2, /*created=*/2, + /*entry_count=*/0); + + ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, CurrentManifests()); + ASSERT_EQ(data_manifests.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto original_data_manifest, + ManifestContainingPath(data_manifests, file_a_->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto concurrent_data_manifest, + ManifestContainingPath(data_manifests, file_c_->file_path)); + ExpectManifestEntriesWithSequenceNumbers( + *original_data_manifest, {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {append_snapshot_id, append_snapshot_id}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}); + ExpectManifestEntriesWithSequenceNumbers( + *concurrent_data_manifest, {file_c_->file_path, file_d_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {concurrent_snapshot_id, concurrent_snapshot_id}, + {concurrent_snapshot_sequence_number, concurrent_snapshot_sequence_number}, + {concurrent_snapshot_sequence_number, concurrent_snapshot_sequence_number}); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(delete_manifests.size(), 2U); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[0], {delete_a->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ExpectManifestEntryContents(delete_manifests[0], {delete_a->file_path}, + {DataFile::Content::kPositionDeletes}); + ExpectManifestEntriesWithSequenceNumbers( + delete_manifests[1], {delete_a2->file_path}, {ManifestStatus::kExisting}, + {delete_snapshot_id}, {delete_snapshot_sequence_number}, + {delete_snapshot_sequence_number}); + ExpectManifestEntryContents(delete_manifests[1], {delete_a2->file_path}, + {DataFile::Content::kEqualityDeletes}); +} + +TEST_P(RewriteManifestsTest, DeleteManifestReplacementConcurrentDeleteFileRemoval) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); + const int64_t append_snapshot_id = append_snapshot->snapshot_id; + const int64_t append_snapshot_sequence_number = append_snapshot->sequence_number; + + auto delete_a = MakeDeleteFile("concurrent-removal-delete-a", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("concurrent-removal-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_delete_snapshot, table_->current_snapshot()); + const int64_t first_delete_snapshot_id = first_delete_snapshot->snapshot_id; + const int64_t first_delete_snapshot_sequence_number = + first_delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto first_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(first_delete_manifests.size(), 1U); + const auto original_delete_manifest = first_delete_manifests[0]; + + auto delete_b = MakeDeleteFile("concurrent-removal-delete-b", 2, file_b_->file_path); + auto delete_c2 = MakeEqualityDeleteFile("concurrent-removal-delete-c2", 3); + ASSERT_THAT(AppendDeleteFiles({delete_b, delete_c2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_delete_snapshot, table_->current_snapshot()); + const int64_t second_delete_snapshot_id = second_delete_snapshot->snapshot_id; + const int64_t second_delete_snapshot_sequence_number = + second_delete_snapshot->sequence_number; + + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a, + WriteExistingManifest("delete-concurrent-removal-a", first_delete_snapshot_id, + {delete_a}, ManifestContent::kDeletes, nullptr, + first_delete_snapshot_sequence_number, + first_delete_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a2, + WriteExistingManifest("delete-concurrent-removal-a2", first_delete_snapshot_id, + {delete_a2}, ManifestContent::kDeletes, nullptr, + first_delete_snapshot_sequence_number, + first_delete_snapshot_sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(original_delete_manifest) + .AddManifest(new_delete_manifest_a) + .AddManifest(new_delete_manifest_a2); + + ASSERT_THAT(RemoveDeleteFiles({delete_b}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto concurrent_snapshot, table_->current_snapshot()); + const int64_t concurrent_snapshot_id = concurrent_snapshot->snapshot_id; + ICEBERG_UNWRAP_OR_FAIL(auto data_before_rewrite, CurrentManifests()); + ICEBERG_UNWRAP_OR_FAIL(auto deletes_before_rewrite, CurrentDeleteManifests()); + ASSERT_EQ(data_before_rewrite.size(), 1U); + ASSERT_EQ(deletes_before_rewrite.size(), 2U); + + EXPECT_THAT(rewrite->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite_snapshot, table_->current_snapshot()); + ValidateSummary(rewrite_snapshot, /*replaced=*/1, /*kept=*/2, /*created=*/2, + /*entry_count=*/0); + + ICEBERG_UNWRAP_OR_FAIL(auto data_manifests, CurrentManifests()); + ASSERT_EQ(data_manifests.size(), 1U); + ExpectManifestEntriesWithSequenceNumbers( + data_manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kAdded, ManifestStatus::kAdded}, + {append_snapshot_id, append_snapshot_id}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}, + {append_snapshot_sequence_number, append_snapshot_sequence_number}); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(delete_manifests.size(), 3U); + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifest_a, + ManifestContainingPath(delete_manifests, delete_a->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifest_a2, + ManifestContainingPath(delete_manifests, delete_a2->file_path)); + ICEBERG_UNWRAP_OR_FAIL(auto delete_manifest_b, + ManifestContainingPath(delete_manifests, delete_b->file_path)); + ExpectManifestEntriesWithSequenceNumbers( + *delete_manifest_a, {delete_a->file_path}, {ManifestStatus::kExisting}, + {first_delete_snapshot_id}, {first_delete_snapshot_sequence_number}, + {first_delete_snapshot_sequence_number}); + ExpectManifestEntryContents(*delete_manifest_a, {delete_a->file_path}, + {DataFile::Content::kPositionDeletes}); + ExpectManifestEntriesWithSequenceNumbers( + *delete_manifest_a2, {delete_a2->file_path}, {ManifestStatus::kExisting}, + {first_delete_snapshot_id}, {first_delete_snapshot_sequence_number}, + {first_delete_snapshot_sequence_number}); + ExpectManifestEntryContents(*delete_manifest_a2, {delete_a2->file_path}, + {DataFile::Content::kEqualityDeletes}); + ExpectManifestEntriesWithSequenceNumbers( + *delete_manifest_b, {delete_b->file_path, delete_c2->file_path}, + {ManifestStatus::kDeleted, ManifestStatus::kExisting}, + {concurrent_snapshot_id, second_delete_snapshot_id}, + {second_delete_snapshot_sequence_number, second_delete_snapshot_sequence_number}, + {second_delete_snapshot_sequence_number, second_delete_snapshot_sequence_number}); + ExpectManifestEntryContents( + *delete_manifest_b, {delete_b->file_path, delete_c2->file_path}, + {DataFile::Content::kPositionDeletes, DataFile::Content::kEqualityDeletes}); +} + +TEST_P(RewriteManifestsTest, DeleteManifestReplacementConflictingDeleteFileRemoval) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + + auto delete_a = MakeDeleteFile("conflicting-removal-delete-a", 1, file_a_->file_path); + auto delete_a2 = MakeEqualityDeleteFile("conflicting-removal-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a, delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto delete_snapshot, table_->current_snapshot()); + const int64_t delete_snapshot_id = delete_snapshot->snapshot_id; + const int64_t delete_snapshot_sequence_number = delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto original_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(original_delete_manifests.size(), 1U); + const auto original_delete_manifest = original_delete_manifests[0]; + + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a, + WriteExistingManifest("delete-conflicting-removal-a", delete_snapshot_id, + {delete_a}, ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest_a2, + WriteExistingManifest("delete-conflicting-removal-a2", delete_snapshot_id, + {delete_a2}, ManifestContent::kDeletes, nullptr, + delete_snapshot_sequence_number, + delete_snapshot_sequence_number)); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(original_delete_manifest) + .AddManifest(new_delete_manifest_a) + .AddManifest(new_delete_manifest_a2); + + ASSERT_THAT(RemoveDeleteFiles({delete_a}), IsOk()); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage(std::format( + "Deleted manifest {} could not be found in the latest snapshot", + original_delete_manifest.manifest_path))); +} + +TEST_P(RewriteManifestsTest, DeleteManifestReplacementFailure) { + if (GetParam() == 1) { + GTEST_SKIP() << "Delete manifests require format version 2 or higher"; + } + + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + + auto delete_a = MakeDeleteFile("failure-delete-a", 1, file_a_->file_path); + ASSERT_THAT(AppendDeleteFiles({delete_a}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first_delete_snapshot, table_->current_snapshot()); + const int64_t first_delete_snapshot_id = first_delete_snapshot->snapshot_id; + const int64_t first_delete_snapshot_sequence_number = + first_delete_snapshot->sequence_number; + + auto delete_a2 = MakeEqualityDeleteFile("failure-delete-a2", 1); + ASSERT_THAT(AppendDeleteFiles({delete_a2}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second_delete_snapshot, table_->current_snapshot()); + const int64_t second_delete_snapshot_id = second_delete_snapshot->snapshot_id; + const int64_t second_delete_snapshot_sequence_number = + second_delete_snapshot->sequence_number; + ICEBERG_UNWRAP_OR_FAIL(auto original_delete_manifests, CurrentDeleteManifests()); + ASSERT_EQ(original_delete_manifests.size(), 2U); + + ICEBERG_UNWRAP_OR_FAIL( + auto new_delete_manifest, + WriteExistingDeleteManifest( + "delete-manifest-file", + {{delete_a, first_delete_snapshot_id, first_delete_snapshot_sequence_number, + first_delete_snapshot_sequence_number}, + {delete_a2, second_delete_snapshot_id, second_delete_snapshot_sequence_number, + second_delete_snapshot_sequence_number}})); + EXPECT_TRUE(FileExists(new_delete_manifest.manifest_path)); + + SetCommitRetryProperties(1); + BindTableWithFailingCommits(/*failures=*/5); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + for (const auto& original_delete_manifest : original_delete_manifests) { + rewrite->DeleteManifest(original_delete_manifest); + } + rewrite->AddManifest(new_delete_manifest); + + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(result, HasErrorMessage("Injected failure")); + EXPECT_TRUE(FileExists(new_delete_manifest.manifest_path)); +} + +INSTANTIATE_TEST_SUITE_P(RewriteManifestVersions, RewriteManifestsTest, + ::testing::Values(1, 2, 3), + [](const ::testing::TestParamInfo& info) { + return "V" + std::to_string(info.param); + }); + +} // namespace iceberg diff --git a/src/iceberg/test/update_test_base.h b/src/iceberg/test/update_test_base.h index e0ea53c3b5..fe8c9705fa 100644 --- a/src/iceberg/test/update_test_base.h +++ b/src/iceberg/test/update_test_base.h @@ -22,13 +22,18 @@ #include #include #include +#include #include +#include +#include #include #include #include "iceberg/arrow/arrow_io_internal.h" #include "iceberg/catalog/memory/in_memory_catalog.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/manifest/manifest_reader.h" #include "iceberg/result.h" #include "iceberg/snapshot.h" #include "iceberg/table.h" @@ -174,6 +179,114 @@ class UpdateTestBase : public ::testing::Test { EXPECT_THAT(result, HasErrorMessage(message)); } + Result> ReadManifestEntries(const ManifestFile& manifest) { + return ReadManifestEntries(std::span(&manifest, 1)); + } + + Result> ReadManifestEntries( + std::span manifests) { + std::vector result; + ICEBERG_ASSIGN_OR_RAISE(auto schema, table_->metadata()->Schema()); + for (const auto& manifest : manifests) { + ICEBERG_ASSIGN_OR_RAISE( + auto spec, table_->metadata()->PartitionSpecById(manifest.partition_spec_id)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, + ManifestReader::Make(manifest, file_io_, schema, spec)); + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->Entries()); + result.insert(result.end(), entries.begin(), entries.end()); + } + return result; + } + + void ExpectManifestEntries(const ManifestFile& manifest, + std::vector expected_paths, + std::vector expected_statuses) { + ASSERT_EQ(expected_statuses.size(), expected_paths.size()); + + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + ASSERT_EQ(entries.size(), expected_paths.size()); + + std::vector> actual; + actual.reserve(entries.size()); + for (const auto& entry : entries) { + ASSERT_NE(entry.data_file, nullptr); + actual.emplace_back(entry.data_file->file_path, entry.status); + } + + std::vector> expected; + expected.reserve(expected_paths.size()); + for (size_t i = 0; i < expected_paths.size(); ++i) { + expected.emplace_back(std::move(expected_paths[i]), expected_statuses[i]); + } + EXPECT_THAT(actual, ::testing::UnorderedElementsAreArray(expected)); + } + + void ExpectManifestEntriesWithSnapshotIds(const ManifestFile& manifest, + std::vector expected_paths, + std::vector expected_statuses, + std::vector expected_snapshot_ids) { + ASSERT_EQ(expected_statuses.size(), expected_paths.size()); + ASSERT_EQ(expected_snapshot_ids.size(), expected_paths.size()); + + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + ASSERT_EQ(entries.size(), expected_paths.size()); + + std::vector> actual; + actual.reserve(entries.size()); + for (const auto& entry : entries) { + ASSERT_NE(entry.data_file, nullptr); + ASSERT_TRUE(entry.snapshot_id.has_value()); + actual.emplace_back(entry.data_file->file_path, entry.status, + entry.snapshot_id.value()); + } + + std::vector> expected; + expected.reserve(expected_paths.size()); + for (size_t i = 0; i < expected_paths.size(); ++i) { + expected.emplace_back(std::move(expected_paths[i]), expected_statuses[i], + expected_snapshot_ids[i]); + } + EXPECT_THAT(actual, ::testing::UnorderedElementsAreArray(expected)); + } + + void ExpectManifestEntriesWithSequenceNumbers( + const ManifestFile& manifest, std::vector expected_paths, + std::vector expected_statuses, + std::vector expected_snapshot_ids, + std::vector expected_data_sequence_numbers, + std::vector expected_file_sequence_numbers) { + ASSERT_EQ(expected_statuses.size(), expected_paths.size()); + ASSERT_EQ(expected_snapshot_ids.size(), expected_paths.size()); + ASSERT_EQ(expected_data_sequence_numbers.size(), expected_paths.size()); + ASSERT_EQ(expected_file_sequence_numbers.size(), expected_paths.size()); + + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + ASSERT_EQ(entries.size(), expected_paths.size()); + + std::vector> + actual; + actual.reserve(entries.size()); + for (const auto& entry : entries) { + ASSERT_NE(entry.data_file, nullptr); + ASSERT_TRUE(entry.snapshot_id.has_value()); + ASSERT_TRUE(entry.sequence_number.has_value()); + ASSERT_TRUE(entry.file_sequence_number.has_value()); + actual.emplace_back(entry.data_file->file_path, entry.status, + entry.snapshot_id.value(), entry.sequence_number.value(), + entry.file_sequence_number.value()); + } + + std::vector> + expected; + expected.reserve(expected_paths.size()); + for (size_t i = 0; i < expected_paths.size(); ++i) { + expected.emplace_back(std::move(expected_paths[i]), expected_statuses[i], + expected_snapshot_ids[i], expected_data_sequence_numbers[i], + expected_file_sequence_numbers[i]); + } + EXPECT_THAT(actual, ::testing::UnorderedElementsAreArray(expected)); + } + TableIdentifier table_ident_; std::string table_location_; std::shared_ptr file_io_; diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 0eaa3c9762..eada4eb903 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -44,6 +44,7 @@ #include "iceberg/update/pending_update.h" #include "iceberg/update/replace_partitions.h" #include "iceberg/update/rewrite_files.h" +#include "iceberg/update/rewrite_manifests.h" #include "iceberg/update/row_delta.h" #include "iceberg/update/set_snapshot.h" #include "iceberg/update/snapshot_manager.h" @@ -702,6 +703,13 @@ Result> Transaction::NewReplacePartitions() { return replace_partitions; } +Result> Transaction::NewRewriteManifests() { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr rewrite_manifests, + RewriteManifests::Make(ctx_->table->name().name, ctx_)); + ICEBERG_RETURN_UNEXPECTED(AddUpdate(rewrite_manifests)); + return rewrite_manifests; +} + Result> Transaction::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr update_statistics, UpdateStatistics::Make(ctx_)); diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 47678b15e3..8e952d5906 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -150,6 +150,9 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this> NewReplacePartitions(); + /// \brief Create a new RewriteManifests to rewrite manifest layout. + Result> NewRewriteManifests(); + /// \brief Create a new SnapshotManager to manage snapshots. Result> NewSnapshotManager(); diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 84a9919097..8a1cf70ed9 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -259,6 +259,7 @@ class OverwriteFiles; class PendingUpdate; class ReplacePartitions; class RewriteFiles; +class RewriteManifests; class RowDelta; class SetSnapshot; class SnapshotManager; diff --git a/src/iceberg/update/rewrite_manifests.cc b/src/iceberg/update/rewrite_manifests.cc new file mode 100644 index 0000000000..d819389aaa --- /dev/null +++ b/src/iceberg/update/rewrite_manifests.cc @@ -0,0 +1,443 @@ +/* + * 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/update/rewrite_manifests.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "iceberg/constants.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/manifest/manifest_list.h" +#include "iceberg/manifest/manifest_reader.h" +#include "iceberg/manifest/manifest_util_internal.h" +#include "iceberg/manifest/manifest_writer.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" // IWYU pragma: keep +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/util/executor_util_internal.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +namespace { + +struct RewriteCandidate { + ManifestFile manifest; + std::shared_ptr spec; +}; + +struct ManifestEntries { + ManifestFile manifest; + std::shared_ptr spec; + std::vector entries; +}; + +struct RewriteWriter { + std::shared_ptr spec; + std::unique_ptr writer; +}; + +} // namespace + +Result> RewriteManifests::Make( + std::string table_name, std::shared_ptr ctx) { + ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); + ICEBERG_PRECHECK(ctx != nullptr, "Cannot create RewriteManifests without a context"); + return std::unique_ptr( + new RewriteManifests(std::move(table_name), std::move(ctx))); +} + +RewriteManifests::RewriteManifests(std::string table_name, + std::shared_ptr ctx) + : SnapshotUpdate(std::move(ctx)), table_name_(std::move(table_name)) {} + +RewriteManifests& RewriteManifests::ClusterBy(ClusterByFunc func) { + ICEBERG_BUILDER_CHECK(static_cast(func), "Cluster function cannot be null"); + cluster_by_func_ = std::move(func); + return *this; +} + +RewriteManifests& RewriteManifests::RewriteIf(RewritePredicate predicate) { + ICEBERG_BUILDER_CHECK(static_cast(predicate), "Rewrite predicate cannot be null"); + predicate_ = std::move(predicate); + return *this; +} + +RewriteManifests& RewriteManifests::DeleteManifest(const ManifestFile& manifest) { + deleted_manifests_.insert(manifest); + return *this; +} + +RewriteManifests& RewriteManifests::AddManifest(const ManifestFile& manifest) { + // Reject added/deleted files unconditionally, matching Java's checkArgument. A + // missing count is treated as non-zero (has_*_files defaults to true), so the + // error is reported at the AddManifest call site rather than deferred to Apply. + ICEBERG_BUILDER_CHECK(!manifest.has_added_files(), + "Cannot add manifest with added files"); + ICEBERG_BUILDER_CHECK(!manifest.has_deleted_files(), + "Cannot add manifest with deleted files"); + ICEBERG_BUILDER_CHECK(!manifest.added_snapshot_id.has_value() || + manifest.added_snapshot_id == kInvalidSnapshotId, + "Snapshot id must be assigned during commit"); + ICEBERG_BUILDER_CHECK(manifest.sequence_number == kInvalidSequenceNumber, + "Sequence number must be assigned during commit"); + + if (can_inherit_snapshot_id()) { + added_manifests_.push_back(manifest); + } else { + // Retain the input so Apply can recreate the copy after retry cleanup. + added_manifests_to_copy_.push_back(manifest); + } + return *this; +} + +Status RewriteManifests::ValidateTargetBranch(const std::string& branch) const { + return NotSupported( + "Cannot commit to branch {}: RewriteManifests does not support branch commits", + branch); +} + +std::string RewriteManifests::operation() { return DataOperation::kReplace; } + +Result> RewriteManifests::Apply( + const TableMetadata& /*metadata_to_update*/, + const std::shared_ptr& snapshot) { + ICEBERG_PRECHECK(snapshot != nullptr, + "Cannot rewrite manifests without a current snapshot"); + + SnapshotReader snapshot_reader(snapshot.get()); + ICEBERG_ASSIGN_OR_RAISE(auto current_manifests, + snapshot_reader.Manifests(ctx_->table->io())); + + auto current_manifest_set = + current_manifests | std::ranges::to>(); + + ICEBERG_RETURN_UNEXPECTED( + ValidateDeletedManifests(current_manifest_set, snapshot->snapshot_id)); + + if (rewritten_added_manifests_.empty()) { + for (const auto& manifest : added_manifests_to_copy_) { + ICEBERG_ASSIGN_OR_RAISE(auto copied_manifest, CopyManifest(manifest)); + rewritten_added_manifests_.push_back(std::move(copied_manifest)); + } + } + + if (RequiresRewrite(current_manifest_set)) { + ICEBERG_RETURN_UNEXPECTED(Rewrite(current_manifests)); + } else { + kept_manifests_ = current_manifests | + std::views::filter([this](const auto& manifest) { + return !rewritten_manifests_.contains(manifest) && + !deleted_manifests_.contains(manifest); + }) | + std::ranges::to(); + } + + ICEBERG_RETURN_UNEXPECTED(ValidateFilesCounts()); + + const std::array created_manifests{std::span(new_manifests_), + std::span(added_manifests_), + std::span(rewritten_added_manifests_)}; + auto manifests = + created_manifests | std::views::join | + std::views::transform([snapshot_id = SnapshotId()](ManifestFile manifest) { + manifest.added_snapshot_id = snapshot_id; + return manifest; + }) | + std::ranges::to(); + manifests.insert(manifests.end(), kept_manifests_.begin(), kept_manifests_.end()); + + manifest_count_summary_ = BuildManifestCountSummary( + manifests, + static_cast(rewritten_manifests_.size() + deleted_manifests_.size())); + return manifests; +} + +std::unordered_map RewriteManifests::Summary() { + summary_.Clear(); + summary_.SetPartitionSummaryLimit(0); + for (const auto& [property, value] : custom_summary_properties_) { + summary_.Set(property, value); + } + summary_.Merge(manifest_count_summary_); + summary_.Set(SnapshotSummaryFields::kEntriesProcessed, std::to_string(entry_count_)); + return summary_.Build(); +} + +void RewriteManifests::SetSummaryProperty(const std::string& property, + const std::string& value) { + custom_summary_properties_[property] = value; + SnapshotUpdate::SetSummaryProperty(property, value); +} + +Status RewriteManifests::CleanUncommitted( + const std::unordered_set& committed) { + DeleteUncommitted(new_manifests_, committed); + DeleteUncommitted(rewritten_added_manifests_, committed); + entry_count_ = 0; + kept_manifests_.clear(); + rewritten_manifests_.clear(); + manifest_count_summary_.Clear(); + return {}; +} + +bool RewriteManifests::RequiresRewrite( + const std::unordered_set& current_manifests) const { + if (!cluster_by_func_) { + // manifests are deleted and added directly so don't perform a rewrite + return false; + } + if (rewritten_manifests_.empty()) { + // nothing yet processed so perform a full rewrite + return true; + } + + // if any processed manifest is not in the current manifest list, perform a full rewrite + return std::ranges::any_of(rewritten_manifests_, [&](const ManifestFile& manifest) { + return !current_manifests.contains(manifest); + }); +} + +bool RewriteManifests::MatchesPredicate(const ManifestFile& manifest) const { + return !predicate_ || predicate_(manifest); +} + +Status RewriteManifests::ValidateDeletedManifests( + const std::unordered_set& current_manifests, + int64_t current_snapshot_id) const { + for (const auto& manifest : deleted_manifests_) { + if (!current_manifests.contains(manifest)) { + return ValidationFailed( + "Deleted manifest {} could not be found in the latest snapshot {}", + manifest.manifest_path, current_snapshot_id); + } + } + return {}; +} + +Status RewriteManifests::ValidateFilesCounts() const { + auto accumulate_active_files = [](const auto& manifests, + int64_t& active_files) -> Status { + for (const auto& manifest : manifests) { + if (!manifest.added_files_count.has_value() || + !manifest.existing_files_count.has_value()) { + return ValidationFailed("Missing file counts in {}", manifest.manifest_path); + } + active_files += manifest.added_files_count.value(); + active_files += manifest.existing_files_count.value(); + } + return {}; + }; + + int64_t created_active_files = 0; + ICEBERG_RETURN_UNEXPECTED( + accumulate_active_files(new_manifests_, created_active_files)); + ICEBERG_RETURN_UNEXPECTED( + accumulate_active_files(added_manifests_, created_active_files)); + ICEBERG_RETURN_UNEXPECTED( + accumulate_active_files(rewritten_added_manifests_, created_active_files)); + + int64_t replaced_active_files = 0; + ICEBERG_RETURN_UNEXPECTED( + accumulate_active_files(rewritten_manifests_, replaced_active_files)); + ICEBERG_RETURN_UNEXPECTED( + accumulate_active_files(deleted_manifests_, replaced_active_files)); + + if (created_active_files != replaced_active_files) { + return ValidationFailed( + "Replaced and created manifests must have the same number of active files: {} " + "(new), {} (old)", + created_active_files, replaced_active_files); + } + return {}; +} + +Result RewriteManifests::CopyManifest(const ManifestFile& manifest) { + ICEBERG_ASSIGN_OR_RAISE(auto schema, base().Schema()); + ICEBERG_ASSIGN_OR_RAISE(auto spec, + base().PartitionSpecById(manifest.partition_spec_id)); + return CopyRewriteManifest(manifest, ctx_->table->io(), schema, spec, SnapshotId(), + ManifestPath(), base().format_version); +} + +Status RewriteManifests::Rewrite(std::span current_manifests) { + ResetRewriteState(); + + using WriterKey = std::pair; + struct WriterKeyHash { + size_t operator()(const WriterKey& key) const { + size_t seed = std::hash{}(key.first); + seed ^= std::hash{}(key.second) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + return seed; + } + }; + + ICEBERG_ASSIGN_OR_RAISE(auto schema, base().Schema()); + std::vector rewrite_candidates; + rewrite_candidates.reserve(current_manifests.size()); + + for (const auto& manifest : current_manifests) { + if (deleted_manifests_.contains(manifest)) { + continue; + } + if (manifest.content == ManifestContent::kDeletes || !MatchesPredicate(manifest)) { + kept_manifests_.push_back(manifest); + continue; + } + + rewritten_manifests_.insert(manifest); + ICEBERG_ASSIGN_OR_RAISE(auto spec, + base().PartitionSpecById(manifest.partition_spec_id)); + rewrite_candidates.push_back( + RewriteCandidate{.manifest = manifest, .spec = std::move(spec)}); + } + + auto file_io = ctx_->table->io(); + ICEBERG_ASSIGN_OR_RAISE( + auto manifest_entries, + ParallelCollect( + plan_executor(), rewrite_candidates, + [&](const RewriteCandidate& candidate) -> Result> { + ICEBERG_ASSIGN_OR_RAISE( + auto reader, ManifestReader::Make(candidate.manifest, file_io, schema, + candidate.spec)); + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntries()); + std::vector result; + result.push_back(ManifestEntries{.manifest = candidate.manifest, + .spec = candidate.spec, + .entries = std::move(entries)}); + return result; + })); + + std::unordered_map writers; + + auto close_writer = + [](RewriteWriter& rewrite_writer) -> Result> { + if (rewrite_writer.writer == nullptr) { + return std::nullopt; + } + ICEBERG_RETURN_UNEXPECTED(rewrite_writer.writer->Close()); + ICEBERG_ASSIGN_OR_RAISE(auto manifest_file, rewrite_writer.writer->ToManifestFile()); + rewrite_writer.writer.reset(); + return manifest_file; + }; + + auto new_writer = [this, &schema](const RewriteWriter& rewrite_writer) { + return ManifestWriter::MakeWriter(base().format_version, SnapshotId(), ManifestPath(), + ctx_->table->io(), rewrite_writer.spec, schema, + ManifestContent::kData); + }; + + // Capture a write failure so all open writers can still be closed below + Status write_status = [&]() -> Status { + for (const auto& manifest_entry : manifest_entries) { + for (const auto& entry : manifest_entry.entries) { + ICEBERG_PRECHECK(entry.data_file != nullptr, + "Manifest entry in {} is missing data_file", + manifest_entry.manifest.manifest_path); + auto key = WriterKey{cluster_by_func_(*entry.data_file), + manifest_entry.manifest.partition_spec_id}; + + auto writer_it = writers.find(key); + if (writer_it == writers.end()) { + auto [inserted_it, _] = + writers.emplace(key, RewriteWriter{.spec = manifest_entry.spec}); + writer_it = inserted_it; + } + + auto& rewrite_writer = writer_it->second; + if (rewrite_writer.writer == nullptr) { + ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); + } else { + ICEBERG_ASSIGN_OR_RAISE(auto length, rewrite_writer.writer->length()); + if (length >= target_manifest_size_bytes()) { + ICEBERG_ASSIGN_OR_RAISE(auto manifest_file, close_writer(rewrite_writer)); + if (manifest_file.has_value()) { + new_manifests_.push_back(std::move(manifest_file).value()); + } + ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); + } + } + + ICEBERG_RETURN_UNEXPECTED(rewrite_writer.writer->WriteExistingEntry(entry)); + ++entry_count_; + } + } + return {}; + }(); + + // Keep the first close error, but continue closing the remaining writers. + std::optional first_close_error; + for (auto& [_, writer] : writers) { + auto manifest_file_result = close_writer(writer); + if (!manifest_file_result.has_value()) { + if (!first_close_error.has_value()) { + first_close_error = manifest_file_result.error(); + } + continue; + } + auto manifest_file = std::move(manifest_file_result).value(); + if (manifest_file.has_value()) { + new_manifests_.push_back(std::move(manifest_file).value()); + } + } + + if (!write_status.has_value()) { + auto error = std::move(write_status).error(); + if (first_close_error.has_value()) { + error.message += "; additionally failed to close manifest writer: "; + error.message += first_close_error->message; + } + return std::unexpected(std::move(error)); + } + if (first_close_error.has_value()) { + return std::unexpected(std::move(first_close_error).value()); + } + return {}; +} + +void RewriteManifests::DeleteUncommitted( + std::vector& manifests, + const std::unordered_set& committed) { + for (const auto& manifest : manifests) { + if (!committed.contains(manifest.manifest_path)) { + std::ignore = DeleteFile(manifest.manifest_path); + } + } + manifests.clear(); +} + +void RewriteManifests::ResetRewriteState() { + DeleteUncommitted(new_manifests_, {}); + entry_count_ = 0; + kept_manifests_.clear(); + rewritten_manifests_.clear(); +} + +} // namespace iceberg diff --git a/src/iceberg/update/rewrite_manifests.h b/src/iceberg/update/rewrite_manifests.h new file mode 100644 index 0000000000..fead9fa71a --- /dev/null +++ b/src/iceberg/update/rewrite_manifests.h @@ -0,0 +1,143 @@ +/* + * 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/update/rewrite_manifests.h + +#include +#include +#include +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" +#include "iceberg/update/snapshot_update.h" + +namespace iceberg { + +/// \brief API for rewriting manifests for a table. +/// +/// This API accumulates manifest files, produces a new snapshot of the table +/// described only by the manifest files that were added, and commits that snapshot +/// as the current. +/// +/// This API can be used to rewrite matching manifests according to a clustering +/// function as well as to replace specific manifests. Manifests that are deleted +/// or added directly are ignored during the rewrite process. The set of active +/// files in replaced manifests must be the same as in new manifests. +/// +/// When committing, these changes will be applied to the latest table snapshot. +/// Commit conflicts will be resolved by applying the changes to the new latest +/// snapshot and reattempting the commit. +class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate { + public: + using ClusterByFunc = std::function; + using RewritePredicate = std::function; + + static Result> Make( + std::string table_name, std::shared_ptr ctx); + + /// \brief Group an existing data file by a cluster key. + /// + /// The cluster key determines which data file will be associated with a + /// particular manifest. All data files with the same cluster key will be written + /// to the same manifest unless the manifest is large and split into multiple + /// files. Manifests deleted via DeleteManifest or added via AddManifest are + /// ignored during the rewrite process. + RewriteManifests& ClusterBy(ClusterByFunc func); + + /// \brief Determine which existing manifest files should be rewritten. + /// + /// Manifests that do not match the predicate are kept as-is. If this is not + /// called and no predicate is set, all manifests will be rewritten. + RewriteManifests& RewriteIf(RewritePredicate predicate); + + /// \brief Delete a manifest file from the table. + RewriteManifests& DeleteManifest(const ManifestFile& manifest); + + /// \brief Add a manifest file to the table. + /// + /// The added manifest cannot contain new or deleted files. + /// + /// By default, the manifest will be rewritten to ensure all entries have + /// explicit snapshot IDs. In that case, it is always the responsibility of the + /// caller to manage the lifecycle of the original manifest. + /// + /// If manifest entries are allowed to inherit the snapshot ID assigned on + /// commit, the manifest should never be deleted manually if the commit succeeds + /// as it will become part of the table metadata and will be cleaned up on + /// expiry. If the manifest gets merged with others while preparing a new + /// snapshot, it will be deleted automatically if this operation is successful. + /// If the commit fails, the manifest will never be deleted and it is up to the + /// caller whether to delete or reuse it. + RewriteManifests& AddManifest(const ManifestFile& manifest); + + std::string operation() override; + + Result> Apply( + const TableMetadata& metadata_to_update, + const std::shared_ptr& snapshot) override; + std::unordered_map Summary() override; + void SetSummaryProperty(const std::string& property, const std::string& value) override; + Status CleanUncommitted(const std::unordered_set& committed) override; + + private: + explicit RewriteManifests(std::string table_name, + std::shared_ptr ctx); + + bool RequiresRewrite(const std::unordered_set& current_manifests) const; + bool MatchesPredicate(const ManifestFile& manifest) const; + Status ValidateDeletedManifests( + const std::unordered_set& current_manifests, + int64_t current_snapshot_id) const; + Status ValidateFilesCounts() const; + + Result CopyManifest(const ManifestFile& manifest); + Status Rewrite(std::span current_manifests); + + void DeleteUncommitted(std::vector& manifests, + const std::unordered_set& committed); + void ResetRewriteState(); + Status ValidateTargetBranch(const std::string& branch) const override; + + private: + std::string table_name_; + ClusterByFunc cluster_by_func_; + RewritePredicate predicate_; + + std::unordered_set deleted_manifests_; + std::vector added_manifests_; + std::vector added_manifests_to_copy_; + std::vector rewritten_added_manifests_; + + std::vector kept_manifests_; + std::vector new_manifests_; + std::unordered_set rewritten_manifests_; + int64_t entry_count_{0}; + + std::unordered_map custom_summary_properties_; + SnapshotSummaryBuilder manifest_count_summary_; +}; + +} // namespace iceberg diff --git a/src/iceberg/update/snapshot_update.h b/src/iceberg/update/snapshot_update.h index 3f06c8b3ed..476c511b35 100644 --- a/src/iceberg/update/snapshot_update.h +++ b/src/iceberg/update/snapshot_update.h @@ -264,6 +264,9 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate { return {}; }; + /// \brief Validate whether this update can target the given branch. + virtual Status ValidateTargetBranch(const std::string& branch) const { return {}; } + /// \brief Apply the update's changes to the given metadata and snapshot. /// /// \param metadata_to_update The base table metadata to apply changes to @@ -295,6 +298,7 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate { std::string ManifestPath(); std::string ManifestListPath(); + OptionalExecutor plan_executor() const { return plan_executor_; } SnapshotSummaryBuilder& summary_builder() { return summary_; } SnapshotSummaryBuilder BuildManifestCountSummary( std::span manifests, int32_t replaced_manifests_count); @@ -332,6 +336,11 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate { return; } + if (auto status = ValidateTargetBranch(branch); !status) [[unlikely]] { + AddError(status.error()); + return; + } + if (auto ref_it = base().refs.find(branch); ref_it != base().refs.end()) { if (ref_it->second->type() != SnapshotRefType::kBranch) { AddError(ErrorKind::kInvalidArgument, From d83d0a8673e77ae639ac3acaee2b3e1740d9e37c Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Sun, 20 Sep 2026 22:47:36 +0800 Subject: [PATCH 2/3] fix: bound manifest rewrite reads and use shared update ownership --- src/iceberg/test/rewrite_manifests_test.cc | 217 +++++++++++++++++++++ src/iceberg/update/rewrite_manifests.cc | 124 +++++++----- src/iceberg/update/rewrite_manifests.h | 2 +- 3 files changed, 290 insertions(+), 53 deletions(-) diff --git a/src/iceberg/test/rewrite_manifests_test.cc b/src/iceberg/test/rewrite_manifests_test.cc index b68f0077bf..86b743e0df 100644 --- a/src/iceberg/test/rewrite_manifests_test.cc +++ b/src/iceberg/test/rewrite_manifests_test.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,7 @@ #include "iceberg/constants.h" #include "iceberg/expression/expressions.h" #include "iceberg/file_io.h" +#include "iceberg/file_reader.h" #include "iceberg/partition_spec.h" #include "iceberg/schema.h" #include "iceberg/table_metadata.h" @@ -56,6 +58,104 @@ namespace iceberg { namespace { +struct ManifestReadState { + std::unordered_set source_paths; + std::mutex mutex; + int open_readers = 0; + int max_open_readers = 0; + int opened_readers = 0; + int64_t unread_entries = 0; + int64_t max_unread_entries = 0; + int64_t processed_entries = 0; + bool fail_after_first_batch = false; +}; + +class TrackingManifestReader : public Reader { + public: + TrackingManifestReader(std::unique_ptr delegate, + std::shared_ptr state) + : delegate_(std::move(delegate)), state_(std::move(state)) {} + + ~TrackingManifestReader() override { Release(); } + + Status Open(const ReaderOptions& options) override { + auto read_options = options; + const bool track = state_->source_paths.contains(options.path); + if (track) { + read_options.properties.Set(ReaderProperties::kBatchSize, int64_t{2}); + } + ICEBERG_RETURN_UNEXPECTED(delegate_->Open(read_options)); + if (track) { + std::lock_guard lock(state_->mutex); + tracked_ = true; + ++state_->opened_readers; + state_->max_open_readers = + std::max(state_->max_open_readers, ++state_->open_readers); + } + return {}; + } + + Status Close() override { + Release(); + return delegate_->Close(); + } + + Result> Next() override { + if (tracked_ && state_->fail_after_first_batch && read_batch_) { + return IOError("Injected streaming manifest read failure"); + } + ICEBERG_ASSIGN_OR_RAISE(auto batch, delegate_->Next()); + if (tracked_ && batch.has_value()) { + read_batch_ = true; + std::lock_guard lock(state_->mutex); + state_->unread_entries += batch->length; + state_->max_unread_entries = + std::max(state_->max_unread_entries, state_->unread_entries); + } + return batch; + } + + Result Schema() override { return delegate_->Schema(); } + + Result> Metadata() override { + return delegate_->Metadata(); + } + + private: + void Release() { + if (tracked_) { + std::lock_guard lock(state_->mutex); + --state_->open_readers; + tracked_ = false; + } + } + + std::unique_ptr delegate_; + std::shared_ptr state_; + bool tracked_ = false; + bool read_batch_ = false; +}; + +class ScopedManifestReadTracking { + public: + explicit ScopedManifestReadTracking(std::shared_ptr state) + : previous_(ReaderFactoryRegistry::GetFactory(FileFormatType::kAvro)) { + ReaderFactoryRegistry::GetFactory(FileFormatType::kAvro) = + [factory = previous_, + state = std::move(state)]() -> Result> { + ICEBERG_ASSIGN_OR_RAISE(auto reader, factory()); + return std::make_unique(std::move(reader), state); + }; + } + + ~ScopedManifestReadTracking() { + ReaderFactoryRegistry::GetFactory(FileFormatType::kAvro) = std::move(previous_); + } + + private: + ReaderFactory previous_; +}; + struct ManifestWriteFailureState { int manifest_output_count = 0; int fail_on_manifest_output = 0; @@ -646,6 +746,123 @@ TEST_P(RewriteManifestsTest, RewriteManifestsAppendedDirectly) { {ManifestStatus::kExisting}, {append_snapshot_id}); } +TEST_P(RewriteManifestsTest, FactoryResultCanCommitDirectly) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto original, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto ctx, + TransactionContext::Make(table_, TransactionKind::kUpdate)); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, + RewriteManifests::Make(table_->name().name, std::move(ctx))); + rewrite->ClusterBy([](const DataFile&) { return ""; }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + ExpectManifestEntriesWithSnapshotIds( + manifests[0], {file_a_->file_path, file_b_->file_path}, + {ManifestStatus::kExisting, ManifestStatus::kExisting}, + {original->snapshot_id, original->snapshot_id}); +} + +TEST_P(RewriteManifestsTest, StreamsEntriesWithBoundedReaders) { + ICEBERG_UNWRAP_OR_FAIL(auto append, table_->NewFastAppend()); + std::vector expected_paths; + for (int i = 0; i < 40; ++i) { + std::vector> files; + for (int j = 0; j < 3; ++j) { + auto file = MakeDataFile(std::format("stream-{}-{}", i, j), 1, 10, i); + files.push_back(file); + expected_paths.push_back(file->file_path); + } + ICEBERG_UNWRAP_OR_FAIL(auto manifest, + WriteAddedManifest(std::format("stream-{}", i), files)); + append->AppendManifest(manifest); + } + ASSERT_THAT(append->Commit(), IsOk()); + + for (bool use_executor : {false, true}) { + SCOPED_TRACE(use_executor); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + auto state = std::make_shared(); + for (const auto& manifest : before) { + state->source_paths.insert(manifest.manifest_path); + } + { + test::ThreadExecutor executor; + ScopedManifestReadTracking tracking(state); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([state](const DataFile& file) { + std::lock_guard lock(state->mutex); + --state->unread_entries; + ++state->processed_entries; + return file.file_path.substr(0, file.file_path.find_last_of('-')); + }); + if (use_executor) { + rewrite->ScanManifestsWith(executor); + } + ASSERT_THAT(rewrite->Commit(), IsOk()); + if (use_executor) { + EXPECT_GT(executor.submit_count(), 0); + } + } + EXPECT_EQ(state->processed_entries, expected_paths.size()); + EXPECT_EQ(state->unread_entries, 0); + EXPECT_LE(state->max_unread_entries, 2); + EXPECT_EQ(state->opened_readers, before.size()); + EXPECT_EQ(state->open_readers, 0); + EXPECT_LT(state->max_open_readers, before.size()); + if (!use_executor) { + EXPECT_EQ(state->max_open_readers, 1); + } + + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + std::vector actual_paths; + for (const auto& manifest : manifests) { + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + for (const auto& entry : entries) { + EXPECT_EQ(entry.status, ManifestStatus::kExisting); + actual_paths.push_back(entry.data_file->file_path); + } + } + EXPECT_THAT(actual_paths, ::testing::UnorderedElementsAreArray(expected_paths)); + } +} + +TEST_P(RewriteManifestsTest, CleansWritersAfterStreamingReadFailure) { + ASSERT_THAT(AppendFiles({file_a_, file_b_, file_c_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + for (bool use_executor : {false, true}) { + SCOPED_TRACE(use_executor); + auto reads = std::make_shared(); + reads->source_paths.insert(before[0].manifest_path); + reads->fail_after_first_batch = true; + auto writes = std::make_shared(); + BindTableWithFileIO(std::make_shared( + file_io_, table_location_ + "/metadata/", writes)); + { + test::ThreadExecutor executor; + ScopedManifestReadTracking tracking(reads); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return ""; }); + if (use_executor) { + rewrite->ScanManifestsWith(executor); + } + auto result = rewrite->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Injected streaming manifest read failure")); + } + EXPECT_EQ(reads->open_readers, 0); + ASSERT_EQ(writes->manifest_outputs.size(), 1U); + EXPECT_TRUE(writes->open_manifest_paths.empty()); + EXPECT_TRUE(writes->deleted_open_files.empty()); + EXPECT_THAT(writes->deleted_files, + ::testing::Contains(writes->manifest_outputs.front())); + EXPECT_FALSE(FileExists(writes->manifest_outputs.front())); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + EXPECT_EQ(after, before); + } +} + TEST_P(RewriteManifestsTest, RewriteManifestsWithScanExecutor) { SetSnapshotIdInheritanceEnabled(); ICEBERG_UNWRAP_OR_FAIL(auto new_manifest, diff --git a/src/iceberg/update/rewrite_manifests.cc b/src/iceberg/update/rewrite_manifests.cc index d819389aaa..f0e4533c27 100644 --- a/src/iceberg/update/rewrite_manifests.cc +++ b/src/iceberg/update/rewrite_manifests.cc @@ -52,10 +52,10 @@ struct RewriteCandidate { std::shared_ptr spec; }; -struct ManifestEntries { +struct ManifestStream { ManifestFile manifest; std::shared_ptr spec; - std::vector entries; + ManifestEntryStreamPtr entries; }; struct RewriteWriter { @@ -65,11 +65,11 @@ struct RewriteWriter { } // namespace -Result> RewriteManifests::Make( +Result> RewriteManifests::Make( std::string table_name, std::shared_ptr ctx) { ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); ICEBERG_PRECHECK(ctx != nullptr, "Cannot create RewriteManifests without a context"); - return std::unique_ptr( + return std::shared_ptr( new RewriteManifests(std::move(table_name), std::move(ctx))); } @@ -318,23 +318,6 @@ Status RewriteManifests::Rewrite(std::span current_manifests RewriteCandidate{.manifest = manifest, .spec = std::move(spec)}); } - auto file_io = ctx_->table->io(); - ICEBERG_ASSIGN_OR_RAISE( - auto manifest_entries, - ParallelCollect( - plan_executor(), rewrite_candidates, - [&](const RewriteCandidate& candidate) -> Result> { - ICEBERG_ASSIGN_OR_RAISE( - auto reader, ManifestReader::Make(candidate.manifest, file_io, schema, - candidate.spec)); - ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntries()); - std::vector result; - result.push_back(ManifestEntries{.manifest = candidate.manifest, - .spec = candidate.spec, - .entries = std::move(entries)}); - return result; - })); - std::unordered_map writers; auto close_writer = @@ -354,39 +337,76 @@ Status RewriteManifests::Rewrite(std::span current_manifests ManifestContent::kData); }; - // Capture a write failure so all open writers can still be closed below - Status write_status = [&]() -> Status { - for (const auto& manifest_entry : manifest_entries) { - for (const auto& entry : manifest_entry.entries) { - ICEBERG_PRECHECK(entry.data_file != nullptr, - "Manifest entry in {} is missing data_file", - manifest_entry.manifest.manifest_path); - auto key = WriterKey{cluster_by_func_(*entry.data_file), - manifest_entry.manifest.partition_spec_id}; - - auto writer_it = writers.find(key); - if (writer_it == writers.end()) { - auto [inserted_it, _] = - writers.emplace(key, RewriteWriter{.spec = manifest_entry.spec}); - writer_it = inserted_it; + auto write_entry = [&](const ManifestEntry& entry, + const ManifestStream& manifest_stream) -> Status { + ICEBERG_PRECHECK(entry.data_file != nullptr, + "Manifest entry in {} is missing data_file", + manifest_stream.manifest.manifest_path); + auto key = WriterKey{cluster_by_func_(*entry.data_file), + manifest_stream.manifest.partition_spec_id}; + + auto writer_it = writers.find(key); + if (writer_it == writers.end()) { + auto [inserted_it, _] = + writers.emplace(key, RewriteWriter{.spec = manifest_stream.spec}); + writer_it = inserted_it; + } + + auto& rewrite_writer = writer_it->second; + if (rewrite_writer.writer == nullptr) { + ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); + } else { + ICEBERG_ASSIGN_OR_RAISE(auto length, rewrite_writer.writer->length()); + if (length >= target_manifest_size_bytes()) { + ICEBERG_ASSIGN_OR_RAISE(auto manifest_file, close_writer(rewrite_writer)); + if (manifest_file.has_value()) { + new_manifests_.push_back(std::move(manifest_file).value()); } + ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); + } + } + + ICEBERG_RETURN_UNEXPECTED(rewrite_writer.writer->WriteExistingEntry(entry)); + ++entry_count_; + return {}; + }; - auto& rewrite_writer = writer_it->second; - if (rewrite_writer.writer == nullptr) { - ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); - } else { - ICEBERG_ASSIGN_OR_RAISE(auto length, rewrite_writer.writer->length()); - if (length >= target_manifest_size_bytes()) { - ICEBERG_ASSIGN_OR_RAISE(auto manifest_file, close_writer(rewrite_writer)); - if (manifest_file.has_value()) { - new_manifests_.push_back(std::move(manifest_file).value()); - } - ICEBERG_ASSIGN_OR_RAISE(rewrite_writer.writer, new_writer(rewrite_writer)); + // Capture read and write failures so all open writers can still be closed below. + Status write_status = [&]() -> Status { + // Open a bounded batch of streams concurrently, then consume entries incrementally. + // Without an executor, keep only one reader open at a time. + constexpr size_t kManifestReadBatchSize = 32; + const size_t batch_size = plan_executor().has_value() ? kManifestReadBatchSize : 1; + auto file_io = ctx_->table->io(); + for (size_t offset = 0; offset < rewrite_candidates.size(); offset += batch_size) { + auto candidates = + std::span(rewrite_candidates) + .subspan(offset, std::min(batch_size, rewrite_candidates.size() - offset)); + ICEBERG_ASSIGN_OR_RAISE( + auto streams, + ParallelCollect( + plan_executor(), candidates, + [&](const RewriteCandidate& candidate) + -> Result> { + ICEBERG_ASSIGN_OR_RAISE( + auto reader, ManifestReader::Make(candidate.manifest, file_io, schema, + candidate.spec)); + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntriesStream()); + std::vector result; + result.push_back(ManifestStream{.manifest = candidate.manifest, + .spec = candidate.spec, + .entries = std::move(entries)}); + return result; + })); + for (auto& stream : streams) { + while (true) { + ICEBERG_ASSIGN_OR_RAISE(auto entry, stream.entries->Next()); + if (!entry.has_value()) { + break; } + ICEBERG_RETURN_UNEXPECTED(write_entry(*entry, stream)); } - - ICEBERG_RETURN_UNEXPECTED(rewrite_writer.writer->WriteExistingEntry(entry)); - ++entry_count_; + stream.entries.reset(); } } return {}; @@ -414,10 +434,10 @@ Status RewriteManifests::Rewrite(std::span current_manifests error.message += "; additionally failed to close manifest writer: "; error.message += first_close_error->message; } - return std::unexpected(std::move(error)); + return ::iceberg::unexpected(std::move(error)); } if (first_close_error.has_value()) { - return std::unexpected(std::move(first_close_error).value()); + return ::iceberg::unexpected(std::move(first_close_error).value()); } return {}; } diff --git a/src/iceberg/update/rewrite_manifests.h b/src/iceberg/update/rewrite_manifests.h index fead9fa71a..882cfa61c0 100644 --- a/src/iceberg/update/rewrite_manifests.h +++ b/src/iceberg/update/rewrite_manifests.h @@ -55,7 +55,7 @@ class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate { using ClusterByFunc = std::function; using RewritePredicate = std::function; - static Result> Make( + static Result> Make( std::string table_name, std::shared_ptr ctx); /// \brief Group an existing data file by a cluster key. From 0f726460b2db706bac6738295dc6950d04c5b7c0 Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Mon, 12 Oct 2026 23:46:27 +0800 Subject: [PATCH 3/3] fix: correct manifest rewrite replay and cache invalidation Normalize legacy replacement sequence numbers on format upgrades, invalidate cached output when rewrite settings change, and compute manifest counts in Summary. Require an unset snapshot ID for manifest reuse, clarify AddManifest ownership, and add regression coverage. --- src/iceberg/manifest/manifest_util.cc | 38 +++- src/iceberg/manifest/manifest_util_internal.h | 10 +- src/iceberg/test/rewrite_manifests_test.cc | 187 +++++++++++++++++- src/iceberg/update/rewrite_manifests.cc | 45 +++-- src/iceberg/update/rewrite_manifests.h | 20 +- 5 files changed, 264 insertions(+), 36 deletions(-) diff --git a/src/iceberg/manifest/manifest_util.cc b/src/iceberg/manifest/manifest_util.cc index c6fe4d453f..5a644a7f32 100644 --- a/src/iceberg/manifest/manifest_util.cc +++ b/src/iceberg/manifest/manifest_util.cc @@ -19,7 +19,9 @@ #include #include +#include +#include "iceberg/file_reader.h" #include "iceberg/inheritable_metadata.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/manifest/manifest_reader.h" @@ -28,10 +30,36 @@ #include "iceberg/result.h" #include "iceberg/schema.h" #include "iceberg/snapshot.h" +#include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" +#include "iceberg/util/string_util.h" namespace iceberg { +Result ReadManifestFormatVersion(const ManifestFile& manifest, + const std::shared_ptr& file_io) { + ReaderOptions options{.path = manifest.manifest_path, + .length = manifest.manifest_length, + .io = file_io, + .projection = std::make_shared( + std::vector{ManifestEntry::kStatus})}; + ICEBERG_ASSIGN_OR_RAISE(auto reader, + ReaderFactoryRegistry::Open(FileFormatType::kAvro, options)); + ICEBERG_ASSIGN_OR_RAISE(auto metadata, reader->Metadata()); + ICEBERG_RETURN_UNEXPECTED(reader->Close()); + auto version = metadata.find("format-version"); + if (version == metadata.end()) { + return int8_t{1}; + } + ICEBERG_ASSIGN_OR_RAISE(auto format_version, + StringUtils::ParseNumber(version->second)); + ICEBERG_CHECK(format_version >= 1 && + format_version <= TableMetadata::kSupportedTableFormatVersion, + "Unsupported manifest format version {} in {}", format_version, + manifest.manifest_path); + return format_version; +} + Result CopyAppendManifest( const ManifestFile& manifest, const std::shared_ptr& file_io, const std::shared_ptr& schema, const std::shared_ptr& spec, @@ -72,6 +100,8 @@ Result CopyRewriteManifest( const ManifestFile& manifest, const std::shared_ptr& file_io, const std::shared_ptr& schema, const std::shared_ptr& spec, int64_t snapshot_id, const std::string& output_path, int8_t format_version) { + ICEBERG_ASSIGN_OR_RAISE(auto source_format_version, + ReadManifestFormatVersion(manifest, file_io)); // Require explicit snapshot IDs and preserve them in the copied entries. ICEBERG_ASSIGN_OR_RAISE(auto inheritable_metadata, InheritableMetadataFactory::Empty()); ICEBERG_ASSIGN_OR_RAISE( @@ -85,7 +115,7 @@ Result CopyRewriteManifest( auto writer, ManifestWriter::MakeWriter(format_version, snapshot_id, output_path, file_io, spec, schema, manifest.content, manifest.first_row_id)); - for (const auto& entry : entries) { + for (auto& entry : entries) { // A rewritten added manifest may only contain existing entries. if (entry.status == ManifestStatus::kAdded) { return ValidationFailed("Cannot add manifest with added files"); @@ -93,6 +123,12 @@ Result CopyRewriteManifest( if (entry.status == ManifestStatus::kDeleted) { return ValidationFailed("Cannot add manifest with deleted files"); } + if (source_format_version == 1) { + // V1 does not persist sequence numbers. Materialize its legacy defaults + // before writing EXISTING entries into a v2 or v3 replacement manifest. + entry.sequence_number = entry.sequence_number.value_or(0); + entry.file_sequence_number = entry.file_sequence_number.value_or(0); + } ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); } ICEBERG_RETURN_UNEXPECTED(writer->Close()); diff --git a/src/iceberg/manifest/manifest_util_internal.h b/src/iceberg/manifest/manifest_util_internal.h index 77de2aaa12..dd88f40d0a 100644 --- a/src/iceberg/manifest/manifest_util_internal.h +++ b/src/iceberg/manifest/manifest_util_internal.h @@ -32,6 +32,11 @@ namespace iceberg { +/// \brief Read the format version recorded in a manifest's file metadata. +/// Missing format-version metadata identifies a v1 manifest. +ICEBERG_EXPORT Result ReadManifestFormatVersion( + const ManifestFile& manifest, const std::shared_ptr& file_io); + /// \brief Copy an append manifest with a new snapshot ID. /// /// This function copies a manifest file that contains only ADDED entries, @@ -54,8 +59,9 @@ ICEBERG_EXPORT Result CopyAppendManifest( SnapshotSummaryBuilder* summary_builder = nullptr); /// \brief Copy a manifest containing only EXISTING entries, preserving their -/// snapshot IDs, sequence numbers, and row IDs. The caller owns output_path and -/// must clean it up if copying or committing fails. +/// snapshot IDs, sequence numbers, and row IDs. Missing v1 sequence numbers are +/// materialized as zero. The caller owns output_path and must clean it up if +/// copying or committing fails. ICEBERG_EXPORT Result CopyRewriteManifest( const ManifestFile& manifest, const std::shared_ptr& file_io, const std::shared_ptr& schema, const std::shared_ptr& spec, diff --git a/src/iceberg/test/rewrite_manifests_test.cc b/src/iceberg/test/rewrite_manifests_test.cc index 86b743e0df..52e7ea3f52 100644 --- a/src/iceberg/test/rewrite_manifests_test.cc +++ b/src/iceberg/test/rewrite_manifests_test.cc @@ -403,6 +403,57 @@ class RewriteManifestsTest : public MinimalUpdateTestBase, EXPECT_THAT(table_->Refresh(), IsOk()); } + void ReplayV1ReplacementAfterUpgrade(bool inherit_snapshot_id) { + if (format_version() != 1) { + GTEST_SKIP() << "This regression starts from a v1 table"; + } + + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + if (inherit_snapshot_id) { + SetSnapshotIdInheritanceEnabled(); + } + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + ICEBERG_UNWRAP_OR_FAIL(auto replacement, WriteExistingManifest("legacy-replacement", + snapshot->snapshot_id, + {file_a_, file_b_})); + if (inherit_snapshot_id) { + replacement.added_snapshot_id.reset(); + } + + ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, txn->NewRewriteManifests()); + rewrite->DeleteManifest(before[0]).AddManifest(replacement); + ASSERT_THAT(rewrite->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kFormatVersion.key()), "2"); + ASSERT_THAT(props->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + ASSERT_THAT(txn->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + ASSERT_EQ(after.size(), 1U); + EXPECT_NE(after[0].manifest_path, replacement.manifest_path); + EXPECT_TRUE(FileExists(replacement.manifest_path)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(after[0])); + ASSERT_EQ(entries.size(), 2U); + for (const auto& entry : entries) { + EXPECT_EQ(entry.status, ManifestStatus::kExisting); + EXPECT_EQ(entry.snapshot_id, snapshot->snapshot_id); + EXPECT_EQ(entry.sequence_number, std::make_optional(0)); + EXPECT_EQ(entry.file_sequence_number, std::make_optional(0)); + } + ICEBERG_UNWRAP_OR_FAIL(auto rewritten_snapshot, table_->current_snapshot()); + ValidateSummary(rewritten_snapshot, /*replaced=*/1, /*kept=*/0, /*created=*/1, + /*entry_count=*/0); + + ICEBERG_UNWRAP_OR_FAIL(auto remove, table_->NewDeleteFiles()); + remove->DeleteFile(file_a_); + ASSERT_THAT(remove->Commit(), IsOk()); + } + void SetManifestMergeEnabled(bool enabled) { ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); props->Set(std::string(TableProperties::kManifestMergeEnabled.key()), @@ -981,6 +1032,96 @@ TEST_P(RewriteManifestsTest, RewriteAfterV2ToV3UpgradeAssignsRowIds) { TableMetadata::kInitialRowId + assigned_rows); } +TEST_P(RewriteManifestsTest, RewritesWhenClusterByChangesAfterApply) { + ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "all"; }); + ICEBERG_UNWRAP_OR_FAIL(auto prepared, rewrite->Apply(*table_->metadata(), snapshot)); + ASSERT_EQ(prepared.size(), 1U); + EXPECT_TRUE(FileExists(prepared[0].manifest_path)); + + rewrite->ClusterBy([](const DataFile& file) { return file.file_path; }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + ASSERT_EQ(after.size(), 2U); + for (const auto& manifest : after) { + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifest)); + EXPECT_EQ(entries.size(), 1U); + } + EXPECT_FALSE(FileExists(prepared[0].manifest_path)); + ICEBERG_UNWRAP_OR_FAIL(auto rewritten_snapshot, table_->current_snapshot()); + ValidateSummary(rewritten_snapshot, /*replaced=*/1, /*kept=*/0, /*created=*/2, + /*entry_count=*/2); +} + +TEST_P(RewriteManifestsTest, RewritesWhenPredicateChangesAfterApply) { + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ASSERT_THAT(AppendFiles({file_b_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->ClusterBy([](const DataFile&) { return "all"; }); + rewrite->RewriteIf([path = before[0].manifest_path](const ManifestFile& manifest) { + return manifest.manifest_path == path; + }); + ICEBERG_UNWRAP_OR_FAIL(auto prepared, rewrite->Apply(*table_->metadata(), snapshot)); + + rewrite->RewriteIf([](const ManifestFile&) { return false; }); + ASSERT_THAT(rewrite->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + EXPECT_THAT(after, ::testing::UnorderedElementsAreArray(before)); + for (const auto& manifest : prepared) { + if (!std::ranges::contains(before, manifest)) { + EXPECT_FALSE(FileExists(manifest.manifest_path)); + } + } + ICEBERG_UNWRAP_OR_FAIL(auto rewritten_snapshot, table_->current_snapshot()); + ValidateSummary(rewritten_snapshot, /*replaced=*/0, /*kept=*/2, /*created=*/0, + /*entry_count=*/0); +} + +TEST_P(RewriteManifestsTest, ReplaysV1ReplacementAfterUpgradeWithSnapshotIdInheritance) { + ReplayV1ReplacementAfterUpgrade(/*inherit_snapshot_id=*/true); +} + +TEST_P(RewriteManifestsTest, + ReplaysV1ReplacementAfterUpgradeWithoutSnapshotIdInheritance) { + ReplayV1ReplacementAfterUpgrade(/*inherit_snapshot_id=*/false); +} + +TEST_P(RewriteManifestsTest, CopiesV1ReplacementAddedAfterFormatUpgrade) { + if (format_version() != 1) { + GTEST_SKIP() << "This regression starts from a v1 manifest"; + } + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ICEBERG_UNWRAP_OR_FAIL( + auto replacement, + WriteExistingManifest("legacy-replacement", snapshot->snapshot_id, {file_a_})); + replacement.added_snapshot_id.reset(); + + ICEBERG_UNWRAP_OR_FAIL(auto props, table_->NewUpdateProperties()); + props->Set(std::string(TableProperties::kFormatVersion.key()), "2"); + ASSERT_THAT(props->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(before[0]).AddManifest(replacement); + ASSERT_THAT(rewrite->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto after, CurrentManifests()); + ASSERT_EQ(after.size(), 1U); + EXPECT_NE(after[0].manifest_path, replacement.manifest_path); + EXPECT_TRUE(FileExists(replacement.manifest_path)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(after[0])); + ASSERT_EQ(entries.size(), 1U); + EXPECT_EQ(entries[0].snapshot_id, snapshot->snapshot_id); + EXPECT_EQ(entries[0].sequence_number, std::make_optional(0)); + EXPECT_EQ(entries[0].file_sequence_number, std::make_optional(0)); +} + TEST_P(RewriteManifestsTest, ReplaceManifestsSeparate) { ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto append_snapshot, table_->current_snapshot()); @@ -1289,9 +1430,6 @@ TEST_P(RewriteManifestsTest, CleansWritersAfterTransactionApplyFailure) { } TEST_P(RewriteManifestsTest, ClosesAndCleansCopiedManifestOnFailure) { - if (format_version() != 1) { - GTEST_SKIP() << "Manifest copying is only required without snapshot ID inheritance"; - } ASSERT_THAT(AppendFiles({file_a_}), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); @@ -1469,13 +1607,8 @@ TEST_P(RewriteManifestsTest, BasicManifestReplacement) { ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); ASSERT_EQ(manifests.size(), 3U); - if (GetParam() == 1) { - EXPECT_NE(manifests[0].manifest_path, manifest_a.manifest_path); - EXPECT_NE(manifests[1].manifest_path, manifest_b.manifest_path); - } else { - EXPECT_EQ(manifests[0].manifest_path, manifest_a.manifest_path); - EXPECT_EQ(manifests[1].manifest_path, manifest_b.manifest_path); - } + EXPECT_NE(manifests[0].manifest_path, manifest_a.manifest_path); + EXPECT_NE(manifests[1].manifest_path, manifest_b.manifest_path); ExpectManifestEntriesWithSnapshotIds(manifests[0], {file_a_->file_path}, {ManifestStatus::kExisting}, {first_snapshot_id}); ExpectManifestEntriesWithSnapshotIds(manifests[1], {file_b_->file_path}, @@ -1563,6 +1696,8 @@ TEST_P(RewriteManifestsTest, BasicManifestReplacementWithSnapshotIdInheritance) ICEBERG_UNWRAP_OR_FAIL( auto manifest_b, WriteExistingManifest("rewrite-inherit-b", first_snapshot_id, {file_b_})); + manifest_a.added_snapshot_id.reset(); + manifest_b.added_snapshot_id.reset(); ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); rewrite->DeleteManifest(first_snapshot_manifests[0]) @@ -1593,6 +1728,37 @@ TEST_P(RewriteManifestsTest, BasicManifestReplacementWithSnapshotIdInheritance) EXPECT_THAT(delete_files->Commit(), IsOk()); } +TEST_P(RewriteManifestsTest, ManifestReplacementWithExplicitInvalidSnapshotIdIsCopied) { + SetSnapshotIdInheritanceEnabled(); + ASSERT_THAT(AppendFiles({file_a_}), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto original_snapshot, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto before, CurrentManifests()); + ASSERT_EQ(before.size(), 1U); + ICEBERG_UNWRAP_OR_FAIL( + auto replacement, + WriteExistingManifest("explicit-invalid-id", original_snapshot->snapshot_id, + {file_a_}, ManifestContent::kData, nullptr, + original_snapshot->sequence_number, + original_snapshot->sequence_number)); + ASSERT_EQ(replacement.added_snapshot_id, kInvalidSnapshotId); + + ICEBERG_UNWRAP_OR_FAIL(auto rewrite, table_->NewRewriteManifests()); + rewrite->DeleteManifest(before[0]).AddManifest(replacement); + ASSERT_THAT(rewrite->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, CurrentManifests()); + ASSERT_EQ(manifests.size(), 1U); + EXPECT_NE(manifests[0].manifest_path, replacement.manifest_path); + EXPECT_TRUE(FileExists(replacement.manifest_path)); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(manifests[0].added_snapshot_id, snapshot->snapshot_id); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadManifestEntries(manifests[0])); + ASSERT_EQ(entries.size(), 1U); + EXPECT_EQ(entries[0].status, ManifestStatus::kExisting); + EXPECT_EQ(entries[0].snapshot_id, original_snapshot->snapshot_id); + EXPECT_EQ(entries[0].sequence_number, original_snapshot->sequence_number); + EXPECT_EQ(entries[0].file_sequence_number, original_snapshot->sequence_number); +} + TEST_P(RewriteManifestsTest, WithMultiplePartitionSpec) { ASSERT_THAT(AppendFiles({file_a_, file_b_}), IsOk()); ICEBERG_UNWRAP_OR_FAIL(auto initial_manifests, CurrentManifests()); @@ -2066,6 +2232,7 @@ TEST_P(RewriteManifestsTest, ManifestReplacementFailureWithSnapshotIdInheritance auto new_manifest, WriteExistingManifest("manifest-file", {{file_a_, first_snapshot_id}, {file_b_, second_snapshot_id}})); + new_manifest.added_snapshot_id.reset(); EXPECT_TRUE(FileExists(new_manifest.manifest_path)); SetCommitRetryProperties(1); diff --git a/src/iceberg/update/rewrite_manifests.cc b/src/iceberg/update/rewrite_manifests.cc index f0e4533c27..3f11d5ed78 100644 --- a/src/iceberg/update/rewrite_manifests.cc +++ b/src/iceberg/update/rewrite_manifests.cc @@ -80,12 +80,14 @@ RewriteManifests::RewriteManifests(std::string table_name, RewriteManifests& RewriteManifests::ClusterBy(ClusterByFunc func) { ICEBERG_BUILDER_CHECK(static_cast(func), "Cluster function cannot be null"); cluster_by_func_ = std::move(func); + rewrite_dirty_ = true; return *this; } RewriteManifests& RewriteManifests::RewriteIf(RewritePredicate predicate) { ICEBERG_BUILDER_CHECK(static_cast(predicate), "Rewrite predicate cannot be null"); predicate_ = std::move(predicate); + rewrite_dirty_ = true; return *this; } @@ -108,7 +110,7 @@ RewriteManifests& RewriteManifests::AddManifest(const ManifestFile& manifest) { ICEBERG_BUILDER_CHECK(manifest.sequence_number == kInvalidSequenceNumber, "Sequence number must be assigned during commit"); - if (can_inherit_snapshot_id()) { + if (can_inherit_snapshot_id() && !manifest.added_snapshot_id.has_value()) { added_manifests_.push_back(manifest); } else { // Retain the input so Apply can recreate the copy after retry cleanup. @@ -141,12 +143,27 @@ Result> RewriteManifests::Apply( ICEBERG_RETURN_UNEXPECTED( ValidateDeletedManifests(current_manifest_set, snapshot->snapshot_id)); - if (rewritten_added_manifests_.empty()) { - for (const auto& manifest : added_manifests_to_copy_) { - ICEBERG_ASSIGN_OR_RAISE(auto copied_manifest, CopyManifest(manifest)); - rewritten_added_manifests_.push_back(std::move(copied_manifest)); + if (base().format_version > 1) { + for (auto it = added_manifests_.begin(); it != added_manifests_.end();) { + ICEBERG_ASSIGN_OR_RAISE(auto format_version, + ReadManifestFormatVersion(*it, ctx_->table->io())); + if (format_version == 1) { + // Reusing a v1 manifest with a new positive commit sequence would leave + // its EXISTING entries without sequence numbers. Copy it on this apply, + // including when the table was upgraded before transaction replay. + added_manifests_to_copy_.push_back(*it); + it = added_manifests_.erase(it); + } else { + ++it; + } } } + for (size_t i = rewritten_added_manifests_.size(); i < added_manifests_to_copy_.size(); + ++i) { + ICEBERG_ASSIGN_OR_RAISE(auto copied_manifest, + CopyManifest(added_manifests_to_copy_[i])); + rewritten_added_manifests_.push_back(std::move(copied_manifest)); + } if (RequiresRewrite(current_manifest_set)) { ICEBERG_RETURN_UNEXPECTED(Rewrite(current_manifests)); @@ -173,9 +190,6 @@ Result> RewriteManifests::Apply( std::ranges::to(); manifests.insert(manifests.end(), kept_manifests_.begin(), kept_manifests_.end()); - manifest_count_summary_ = BuildManifestCountSummary( - manifests, - static_cast(rewritten_manifests_.size() + deleted_manifests_.size())); return manifests; } @@ -185,7 +199,13 @@ std::unordered_map RewriteManifests::Summary() { for (const auto& [property, value] : custom_summary_properties_) { summary_.Set(property, value); } - summary_.Merge(manifest_count_summary_); + summary_.Set(SnapshotSummaryFields::kManifestsCreated, + std::to_string(new_manifests_.size() + added_manifests_.size() + + rewritten_added_manifests_.size())); + summary_.Set(SnapshotSummaryFields::kManifestsKept, + std::to_string(kept_manifests_.size())); + summary_.Set(SnapshotSummaryFields::kManifestsReplaced, + std::to_string(rewritten_manifests_.size() + deleted_manifests_.size())); summary_.Set(SnapshotSummaryFields::kEntriesProcessed, std::to_string(entry_count_)); return summary_.Build(); } @@ -203,7 +223,6 @@ Status RewriteManifests::CleanUncommitted( entry_count_ = 0; kept_manifests_.clear(); rewritten_manifests_.clear(); - manifest_count_summary_.Clear(); return {}; } @@ -213,8 +232,8 @@ bool RewriteManifests::RequiresRewrite( // manifests are deleted and added directly so don't perform a rewrite return false; } - if (rewritten_manifests_.empty()) { - // nothing yet processed so perform a full rewrite + if (rewrite_dirty_ || rewritten_manifests_.empty()) { + // Configuration changes or an empty cache require a full rewrite. return true; } @@ -439,6 +458,7 @@ Status RewriteManifests::Rewrite(std::span current_manifests if (first_close_error.has_value()) { return ::iceberg::unexpected(std::move(first_close_error).value()); } + rewrite_dirty_ = false; return {}; } @@ -458,6 +478,7 @@ void RewriteManifests::ResetRewriteState() { entry_count_ = 0; kept_manifests_.clear(); rewritten_manifests_.clear(); + rewrite_dirty_ = true; } } // namespace iceberg diff --git a/src/iceberg/update/rewrite_manifests.h b/src/iceberg/update/rewrite_manifests.h index 882cfa61c0..b027467fca 100644 --- a/src/iceberg/update/rewrite_manifests.h +++ b/src/iceberg/update/rewrite_manifests.h @@ -80,17 +80,15 @@ class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate { /// /// The added manifest cannot contain new or deleted files. /// - /// By default, the manifest will be rewritten to ensure all entries have - /// explicit snapshot IDs. In that case, it is always the responsibility of the - /// caller to manage the lifecycle of the original manifest. + /// The manifest is copied unless snapshot ID inheritance is enabled and its + /// snapshot ID is unset. A copy may also be required for format compatibility. + /// Copies preserve existing entry snapshot IDs. Added manifests are not merged + /// with other manifests. /// - /// If manifest entries are allowed to inherit the snapshot ID assigned on - /// commit, the manifest should never be deleted manually if the commit succeeds - /// as it will become part of the table metadata and will be cleaned up on - /// expiry. If the manifest gets merged with others while preparing a new - /// snapshot, it will be deleted automatically if this operation is successful. - /// If the commit fails, the manifest will never be deleted and it is up to the - /// caller whether to delete or reuse it. + /// When a copy is made, the caller manages the original manifest and this update + /// manages the copy. When the original is reused and the commit succeeds, it + /// becomes part of the table metadata and must not be deleted manually; it will + /// be cleaned up on expiry. This update does not delete the original on failure. RewriteManifests& AddManifest(const ManifestFile& manifest); std::string operation() override; @@ -125,6 +123,7 @@ class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate { std::string table_name_; ClusterByFunc cluster_by_func_; RewritePredicate predicate_; + bool rewrite_dirty_{true}; std::unordered_set deleted_manifests_; std::vector added_manifests_; @@ -137,7 +136,6 @@ class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate { int64_t entry_count_{0}; std::unordered_map custom_summary_properties_; - SnapshotSummaryBuilder manifest_count_summary_; }; } // namespace iceberg