From ac8563408a379e72bc7c1207a2ab016d7e9ae157 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Thu, 12 Nov 2026 22:37:49 +0800 Subject: [PATCH] fix: align merging snapshot updates and manifest managers with Java Match Java manifest pruning, filtering, bin merging, and per-snapshot delete conflict validation. Preserve retry summaries and sequence inheritance, and use the configured executor while retaining result order and cleanup semantics. --- src/iceberg/deletes/dv_util.cc | 33 +- src/iceberg/deletes/dv_util_internal.h | 3 +- src/iceberg/inheritable_metadata.cc | 3 +- src/iceberg/manifest/manifest_entry.cc | 29 + src/iceberg/manifest/manifest_entry.h | 9 +- .../manifest/manifest_filter_manager.cc | 998 +++++------ .../manifest/manifest_filter_manager.h | 175 +- .../manifest/manifest_merge_manager.cc | 361 ++-- src/iceberg/manifest/manifest_merge_manager.h | 79 +- .../test/manifest_filter_manager_test.cc | 392 ++++- .../test/manifest_merge_manager_test.cc | 248 +++ .../test/merging_snapshot_update_test.cc | 297 +++- src/iceberg/test/partition_value_test.cc | 32 + src/iceberg/update/merging_snapshot_update.cc | 1563 +++++++++-------- src/iceberg/update/merging_snapshot_update.h | 33 +- src/iceberg/util/partition_value_util.h | 23 +- 16 files changed, 2650 insertions(+), 1628 deletions(-) diff --git a/src/iceberg/deletes/dv_util.cc b/src/iceberg/deletes/dv_util.cc index 670ccfedbc..faf7bb17f1 100644 --- a/src/iceberg/deletes/dv_util.cc +++ b/src/iceberg/deletes/dv_util.cc @@ -17,6 +17,7 @@ * under the License. */ +#include #include #include #include @@ -39,17 +40,41 @@ #include "iceberg/result.h" #include "iceberg/util/content_file_util.h" #include "iceberg/util/macros.h" +#include "iceberg/util/task_group.h" #include "iceberg/version.h" namespace iceberg { Result>> DVUtil::MergeAndWriteDVs( std::span groups, std::string_view output_path, - const std::shared_ptr& io) { + const std::shared_ptr& io, OptionalExecutor executor) { if (groups.empty()) { return std::vector>{}; } + // Read all duplicate DVs in parallel, retaining their original order. + std::vector> duplicate_dvs; + for (const auto& group : groups) { + duplicate_dvs.insert(duplicate_dvs.end(), group.delete_files.begin(), + group.delete_files.end()); + } + std::vector duplicated_dv_positions(duplicate_dvs.size()); + std::atomic failed{false}; + auto tasks = TaskGroup().SetExecutor(executor); + for (size_t index = 0; index < duplicate_dvs.size(); ++index) { + tasks.Submit([&, index]() -> Status { + if (failed.load()) return {}; + auto positions = ReadDV(duplicate_dvs[index], io); + if (!positions) { + failed.store(true); + return ::iceberg::unexpected(std::move(positions.error())); + } + duplicated_dv_positions[index] = std::move(positions.value()); + return {}; + }); + } + ICEBERG_RETURN_UNEXPECTED(std::move(tasks).Run()); + ICEBERG_ASSIGN_OR_RAISE( auto writer, DVWriter::Make(DVWriterOptions{ @@ -58,11 +83,11 @@ Result>> DVUtil::MergeAndWriteDVs( .load_previous_deletes = [](std::string_view) -> Result> { return std::nullopt; }})); + size_t index = 0; for (const auto& group : groups) { PositionDeleteIndex merged; - for (const auto& delete_file : group.delete_files) { - ICEBERG_ASSIGN_OR_RAISE(auto index, DVUtil::ReadDV(delete_file, io)); - merged.Merge(index); + for (size_t pos = 0; pos < group.delete_files.size(); ++pos) { + merged.Merge(duplicated_dv_positions[index++]); } ICEBERG_RETURN_UNEXPECTED( writer->Delete(group.referenced_data_file, merged, group.spec, group.partition)); diff --git a/src/iceberg/deletes/dv_util_internal.h b/src/iceberg/deletes/dv_util_internal.h index 8c28740417..3750250275 100644 --- a/src/iceberg/deletes/dv_util_internal.h +++ b/src/iceberg/deletes/dv_util_internal.h @@ -36,6 +36,7 @@ #include "iceberg/result.h" #include "iceberg/row/partition_values.h" #include "iceberg/type_fwd.h" +#include "iceberg/util/executor.h" namespace iceberg { @@ -50,7 +51,7 @@ class ICEBERG_EXPORT DVUtil { public: static Result>> MergeAndWriteDVs( std::span groups, std::string_view output_path, - const std::shared_ptr& io); + const std::shared_ptr& io, OptionalExecutor executor = std::nullopt); static Result ReadDV(const std::shared_ptr& delete_file, const std::shared_ptr& io); diff --git a/src/iceberg/inheritable_metadata.cc b/src/iceberg/inheritable_metadata.cc index e36b075edb..defb3a47dc 100644 --- a/src/iceberg/inheritable_metadata.cc +++ b/src/iceberg/inheritable_metadata.cc @@ -65,8 +65,7 @@ Status BaseInheritableMetadata::Apply(ManifestEntry& entry) { if (entry.data_file) { entry.data_file->partition_spec_id = spec_id_; entry.data_file->data_sequence_number = entry.sequence_number; - // TODO(gangwu): Also propagate file_sequence_number and manifest_location - // when C++ adds consumers that need Java-style inherited file metadata. + entry.data_file->manifest_location = manifest_location_; } else { return InvalidManifest("Manifest entry has no data file"); } diff --git a/src/iceberg/manifest/manifest_entry.cc b/src/iceberg/manifest/manifest_entry.cc index 91fba88d4a..0f64d7bd57 100644 --- a/src/iceberg/manifest/manifest_entry.cc +++ b/src/iceberg/manifest/manifest_entry.cc @@ -20,6 +20,7 @@ #include "iceberg/manifest/manifest_entry.h" #include +#include #include #include "iceberg/schema_field.h" @@ -27,6 +28,34 @@ namespace iceberg { +bool DataFile::operator==(const DataFile& other) const { + return std::tie(content, file_path, file_format, partition, record_count, + file_size_in_bytes, column_sizes, value_counts, null_value_counts, + nan_value_counts, lower_bounds, upper_bounds, key_metadata, + split_offsets, equality_ids, sort_order_id, first_row_id, + referenced_data_file, content_offset, content_size_in_bytes, + partition_spec_id, data_sequence_number) == + std::tie(other.content, other.file_path, other.file_format, other.partition, + other.record_count, other.file_size_in_bytes, other.column_sizes, + other.value_counts, other.null_value_counts, other.nan_value_counts, + other.lower_bounds, other.upper_bounds, other.key_metadata, + other.split_offsets, other.equality_ids, other.sort_order_id, + other.first_row_id, other.referenced_data_file, other.content_offset, + other.content_size_in_bytes, other.partition_spec_id, + other.data_sequence_number); +} + +std::shared_ptr DataFile::CopyWithoutStats() const { + auto copy = std::make_shared(*this); + copy->column_sizes.clear(); + copy->value_counts.clear(); + copy->null_value_counts.clear(); + copy->nan_value_counts.clear(); + copy->lower_bounds.clear(); + copy->upper_bounds.clear(); + return copy; +} + bool ManifestEntry::operator==(const ManifestEntry& other) const { return status == other.status && snapshot_id == other.snapshot_id && sequence_number == other.sequence_number && diff --git a/src/iceberg/manifest/manifest_entry.h b/src/iceberg/manifest/manifest_entry.h index 8b1e550a81..9690e81de3 100644 --- a/src/iceberg/manifest/manifest_entry.h +++ b/src/iceberg/manifest/manifest_entry.h @@ -187,6 +187,12 @@ struct ICEBERG_EXPORT DataFile { /// the DataFile struct. std::optional data_sequence_number; + /// \brief Location of the manifest from which this file was read (not serialized). + std::optional manifest_location; + + /// \brief Copy this file without column-level metrics, retaining file metadata. + std::shared_ptr CopyWithoutStats() const; + static constexpr int32_t kContentFieldId = 134; inline static const SchemaField kContent = SchemaField::MakeOptional( kContentFieldId, "content", int32(), @@ -301,7 +307,8 @@ struct ICEBERG_EXPORT DataFile { SchemaField::MakeOptional(kContentSizeFieldId, "content_size_in_bytes", int64(), "The length of referenced content stored in the file"); - bool operator==(const DataFile& other) const = default; + /// Value equality excludes transient manifest provenance. + bool operator==(const DataFile& other) const; /// \brief Get the schema of the data file with the given partition type. static std::shared_ptr Type(std::shared_ptr partition_type); diff --git a/src/iceberg/manifest/manifest_filter_manager.cc b/src/iceberg/manifest/manifest_filter_manager.cc index 49fec04618..d0f8499691 100644 --- a/src/iceberg/manifest/manifest_filter_manager.cc +++ b/src/iceberg/manifest/manifest_filter_manager.cc @@ -19,10 +19,8 @@ #include "iceberg/manifest/manifest_filter_manager.h" -#include +#include #include -#include -#include #include "iceberg/expression/expression.h" #include "iceberg/expression/expressions.h" @@ -31,25 +29,18 @@ #include "iceberg/expression/residual_evaluator.h" #include "iceberg/expression/strict_metrics_evaluator.h" #include "iceberg/file_io.h" -#include "iceberg/manifest/manifest_entry.h" -#include "iceberg/manifest/manifest_list.h" +#include "iceberg/logging/log_macros.h" #include "iceberg/manifest/manifest_reader.h" #include "iceberg/partition_spec.h" -#include "iceberg/result.h" -#include "iceberg/snapshot.h" #include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" +#include "iceberg/util/task_group.h" namespace iceberg { - namespace { using PartitionSpecsById = ManifestFilterManager::PartitionSpecsById; -bool HasRowFilterExpression(const std::shared_ptr& expr) { - return expr != nullptr && expr->op() != Expression::Operation::kFalse; -} - Result> PartitionSpecById( const PartitionSpecsById& specs_by_id, int32_t spec_id) { auto iter = specs_by_id.find(spec_id); @@ -59,71 +50,151 @@ Result> PartitionSpecById( return iter->second; } -Result FormatPartitionPath(const PartitionSpecsById& specs_by_id, - const DataFile& file, int32_t spec_id) { - ICEBERG_ASSIGN_OR_RAISE(auto spec, PartitionSpecById(specs_by_id, spec_id)); - return spec->PartitionPath(file.partition); -} - -void AddDeletedFileToManager(ManifestContent manifest_content, DataFileSet& data_files, - DeleteFileSet& delete_files, - std::vector>& deleted_files, - DataFileSet& deleted_data_file_set, - DeleteFileSet& deleted_file_set, - const std::shared_ptr& file) { - if (file == nullptr) { - return; - } - - bool inserted; - if (manifest_content == ManifestContent::kData) { - data_files.insert(file); - inserted = deleted_data_file_set.insert(file).second; - } else { - delete_files.insert(file); - inserted = deleted_file_set.insert(file).second; +// ManifestFileUtil.canContainAny: partition summaries are inclusive and spec-local. +Result CanContainAny(const ManifestFile& manifest, const PartitionSet& partitions, + const Schema& table_schema, + const PartitionSpecsById& specs_by_id) { + // C++ represents both absent summaries and the unpartitioned empty tuple as {}. + if (manifest.partitions.empty()) { + return true; } - if (inserted) { - deleted_files.push_back(file); + ICEBERG_ASSIGN_OR_RAISE(auto spec, + PartitionSpecById(specs_by_id, manifest.partition_spec_id)); + ICEBERG_ASSIGN_OR_RAISE(auto partition_type, spec->PartitionType(table_schema)); + ICEBERG_PRECHECK(manifest.partitions.size() <= partition_type->fields().size(), + "Invalid partition summaries for manifest: {}", + manifest.manifest_path); + for (const auto& [spec_id, partition] : partitions) { + if (spec_id != manifest.partition_spec_id || + partition.num_fields() != manifest.partitions.size()) { + continue; + } + bool can_contain = true; + for (size_t pos = 0; pos < manifest.partitions.size(); ++pos) { + const auto& value = partition.values()[pos]; + const auto& summary = manifest.partitions[pos]; + auto type = + std::static_pointer_cast(partition_type->fields()[pos].type()); + if (value.IsNull()) { + can_contain = summary.contains_null; + } else if (value.IsNaN()) { + can_contain = summary.contains_nan.value_or(true); + } else if (type->type_id() == TypeId::kUnknown) { + can_contain = true; + } else if (!summary.lower_bound.has_value()) { + can_contain = false; + } else { + ICEBERG_ASSIGN_OR_RAISE(auto lower_bound, + Literal::Deserialize(*summary.lower_bound, type)); + if (value.value().index() != lower_bound.value().index()) { + can_contain = false; + } else { + ICEBERG_PRECHECK(summary.upper_bound.has_value(), + "Missing upper partition bound: {}", manifest.manifest_path); + ICEBERG_ASSIGN_OR_RAISE(auto upper_bound, + Literal::Deserialize(*summary.upper_bound, type)); + ICEBERG_ASSIGN_OR_RAISE(auto typed_value, value.CastTo(type)); + can_contain = !(typed_value < lower_bound || typed_value > upper_bound); + } + } + if (!can_contain) break; + } + if (can_contain) return true; } + return false; } -bool AddDeletedFileToManifest(ManifestContent manifest_content, - std::vector>& deleted_files, - DataFileSet& deleted_data_file_set, - DeleteFileSet& deleted_file_set, - const std::shared_ptr& file) { - if (file == nullptr) { - return false; - } - bool inserted = manifest_content == ManifestContent::kData - ? deleted_data_file_set.insert(file).second - : deleted_file_set.insert(file).second; - if (inserted) { - deleted_files.push_back(file); - } - return inserted; +bool InsertFile(ManifestContent content, DataFileSet& data_files, + DeleteFileSet& delete_files, const std::shared_ptr& file) { + return content == ManifestContent::kData ? data_files.insert(file).second + : delete_files.insert(file).second; } } // namespace +// First partially evaluate the expression using the partition tuple, then check the +// remaining expression using metrics evaluators, cached for each partition. +class ManifestFilterManager::PartitionAndMetricsEvaluator { + public: + static Result> Make( + std::shared_ptr table_schema, const PartitionSpec& spec, + std::shared_ptr expr, bool case_sensitive) { + ICEBERG_ASSIGN_OR_RAISE( + auto residual_evaluator, + ResidualEvaluator::Make(std::move(expr), spec, *table_schema, case_sensitive)); + return std::unique_ptr(new PartitionAndMetricsEvaluator( + std::move(table_schema), std::move(residual_evaluator), case_sensitive)); + } + + Result RowsMightMatch(const DataFile& file) { + ICEBERG_ASSIGN_OR_RAISE(auto evaluators, MetricsEvaluators(file)); + return evaluators->first->Evaluate(file); + } + + Result RowsMustMatch(const DataFile& file) { + ICEBERG_ASSIGN_OR_RAISE(auto evaluators, MetricsEvaluators(file)); + return evaluators->second->Evaluate(file); + } + + private: + using Evaluators = std::pair, + std::unique_ptr>; + PartitionAndMetricsEvaluator(std::shared_ptr table_schema, + std::unique_ptr residual_evaluator, + bool case_sensitive) + : table_schema_(std::move(table_schema)), + residual_evaluator_(std::move(residual_evaluator)), + case_sensitive_(case_sensitive) {} + + Result MetricsEvaluators(const DataFile& file) { + auto iter = metrics_evaluators_.find(file.partition); + if (iter == metrics_evaluators_.end()) { + // ResidualEvaluator replaces predicates proved true/false by partition + // projections. The remaining predicates need evaluation using file metrics. + ICEBERG_ASSIGN_OR_RAISE(auto residual, + residual_evaluator_->ResidualFor(file.partition)); + ICEBERG_ASSIGN_OR_RAISE( + auto inclusive, + InclusiveMetricsEvaluator::Make(residual, *table_schema_, case_sensitive_)); + ICEBERG_ASSIGN_OR_RAISE(auto strict, StrictMetricsEvaluator::Make( + residual, table_schema_, case_sensitive_)); + // Copy the partition: readers may reuse their partition container. + iter = metrics_evaluators_ + .emplace(file.partition, + Evaluators{std::move(inclusive), std::move(strict)}) + .first; + } + return &iter->second; + } + + std::shared_ptr table_schema_; + std::unique_ptr residual_evaluator_; + bool case_sensitive_; + std::unordered_map + metrics_evaluators_; +}; + Result> ManifestFilterManager::Make( ManifestContent content, std::shared_ptr file_io, - std::function delete_file) { + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier) { ICEBERG_PRECHECK(file_io != nullptr, "FileIO cannot be null"); return std::unique_ptr( - new ManifestFilterManager(content, std::move(file_io), std::move(delete_file))); + new ManifestFilterManager(content, std::move(file_io), std::move(delete_file), + std::move(worker_pool_supplier))); } ManifestFilterManager::ManifestFilterManager( ManifestContent content, std::shared_ptr file_io, - std::function delete_file) + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier) : manifest_content_(content), file_io_(std::move(file_io)), delete_file_(std::move(delete_file)), - delete_expr_(Expressions::AlwaysFalse()) { - ICEBERG_DCHECK(file_io_, "FileIO cannot be null"); - if (delete_file_ == nullptr) { + delete_expression_(Expressions::AlwaysFalse()), + worker_pool_supplier_(std::move(worker_pool_supplier)) { + if (!delete_file_) { delete_file_ = [this](const std::string& location) { return file_io_->DeleteFile(location); }; @@ -132,87 +203,34 @@ ManifestFilterManager::ManifestFilterManager( ManifestFilterManager::~ManifestFilterManager() = default; -Status ManifestFilterManager::DeleteByRowFilter(std::shared_ptr expr) { - ICEBERG_PRECHECK(expr != nullptr, "Cannot delete files using filter: null"); - ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); - ICEBERG_ASSIGN_OR_RAISE(delete_expr_, Or::MakeFolded(delete_expr_, std::move(expr))); - manifest_evaluator_cache_.clear(); - residual_evaluator_cache_.clear(); - return {}; -} - -void ManifestFilterManager::CaseSensitive(bool case_sensitive) { - case_sensitive_ = case_sensitive; - manifest_evaluator_cache_.clear(); - residual_evaluator_cache_.clear(); -} - -Status ManifestFilterManager::DeleteFile(std::string_view path) { - ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); - delete_paths_.insert(std::string(path)); - return {}; -} +void ManifestFilterManager::FailAnyDelete() { fail_any_delete_ = true; } -Status ManifestFilterManager::DeleteFile(std::shared_ptr file) { - ICEBERG_PRECHECK(file != nullptr, "Cannot delete file: null"); - ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); - if (manifest_content_ == ManifestContent::kData) { - data_files_.insert(file); - data_files_to_delete_.insert(std::move(file)); - } else { - delete_files_.insert(file); - delete_files_to_delete_.insert(std::move(file)); - } - return {}; +void ManifestFilterManager::FailMissingDeletePaths() { + fail_missing_delete_paths_ = true; } const DataFileSet& ManifestFilterManager::FilesToBeDeleted() const { return data_files_; } -const std::vector>& ManifestFilterManager::DeletedFiles() - const { - return deleted_files_; +const DeleteFileSet& ManifestFilterManager::DeleteFilesToBeDeleted() const { + return delete_files_; } -Result ManifestFilterManager::BuildSummary( - const std::vector& manifests, - const PartitionSpecsById& specs_by_id) const { - SnapshotSummaryBuilder summary; - for (const auto& manifest : manifests) { - auto deleted_iter = filtered_manifest_to_deleted_files_.find(manifest); - if (deleted_iter == filtered_manifest_to_deleted_files_.end()) { - continue; - } - - ICEBERG_ASSIGN_OR_RAISE(auto spec, - PartitionSpecById(specs_by_id, manifest.partition_spec_id)); - for (const auto& file : deleted_iter->second.files) { - if (file != nullptr) { - ICEBERG_RETURN_UNEXPECTED(summary.DeletedFile(*spec, *file)); - } - } - } - summary.IncrementDuplicateDeletes(duplicate_deletes_count_); - return summary; +Status ManifestFilterManager::DeleteByRowFilter(std::shared_ptr expr) { + ICEBERG_PRECHECK(expr != nullptr, "Cannot delete files using filter: null"); + ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); + ICEBERG_ASSIGN_OR_RAISE(delete_expression_, + Or::MakeFolded(delete_expression_, std::move(expr))); + all_deletes_reference_manifests_ = false; + return {}; } Status ManifestFilterManager::DropPartition(int32_t spec_id, PartitionValues partition) { ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); drop_partitions_.add(spec_id, std::move(partition)); + all_deletes_reference_manifests_ = false; return {}; } -void ManifestFilterManager::FailMissingDeletePaths() { - fail_missing_delete_paths_ = true; -} - -void ManifestFilterManager::FailAnyDelete() { fail_any_delete_ = true; } - -bool ManifestFilterManager::ContainsDeletes() const { - return HasRowFilterExpression(delete_expr_) || !delete_paths_.empty() || - !data_files_to_delete_.empty() || !delete_files_to_delete_.empty() || - !drop_partitions_.empty(); -} - Status ManifestFilterManager::DropDeleteFilesOlderThan(int64_t sequence_number) { ICEBERG_PRECHECK(sequence_number >= 0, "Invalid minimum data sequence number: {}", sequence_number); @@ -220,279 +238,213 @@ Status ManifestFilterManager::DropDeleteFilesOlderThan(int64_t sequence_number) return {}; } -void ManifestFilterManager::RemoveDanglingDeletesFor(const DataFileSet& deleted_files) { - std::unordered_set removed_data_file_paths; - for (const auto& file : deleted_files) { - if (file != nullptr) { - removed_data_file_paths.insert(file->file_path); - } - } - removed_data_file_paths_ = std::move(removed_data_file_paths); +void ManifestFilterManager::CaseSensitive(bool new_case_sensitive) { + case_sensitive_ = new_case_sensitive; } -Result ManifestFilterManager::CanContainDroppedFiles(const ManifestFile&) const { - // TODO(Guotao): prune object deletes by partition once manifest partition - // summary checks are available. - return !delete_paths_.empty() || !data_files_to_delete_.empty() || - !delete_files_to_delete_.empty() || !removed_data_file_paths_.empty(); +void ManifestFilterManager::RemoveDanglingDeletesFor(const DataFileSet& data_files) { + removed_data_file_paths_.clear(); + for (const auto& file : data_files) { + removed_data_file_paths_.insert(file->file_path); + } } -Result ManifestFilterManager::CanContainDroppedPartitions( - const ManifestFile& manifest) const { - if (drop_partitions_.empty()) return false; - // TODO(Guotao): Use partition_summaries bounds to skip manifests that cannot - // contain any dropped partition, instead of only matching partition spec IDs. - // Only manifests whose partition spec matches a registered drop can contain - // entries for that partition. PartitionKey is pair. - int32_t spec_id = manifest.partition_spec_id; - for (const auto& key : drop_partitions_) { - if (key.first == spec_id) return true; +Status ManifestFilterManager::DeleteFile(std::shared_ptr file) { + ICEBERG_PRECHECK(file != nullptr, "Cannot delete file: null"); + ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); + if (!file->manifest_location.has_value()) { + all_deletes_reference_manifests_ = false; + } else { + manifests_with_deletes_.insert(*file->manifest_location); } - return false; + InsertFile(manifest_content_, data_files_, delete_files_, file); + // Files built without inherited metadata use the initial spec, as Java builders do. + delete_file_partitions_.add(file->partition_spec_id.value_or(0), file->partition); + return {}; } -Result ManifestFilterManager::CanContainExpressionDeletes( - const ManifestFile& manifest, const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id) { - if (!HasRowFilterExpression(delete_expr_)) return false; - int32_t spec_id = manifest.partition_spec_id; - ICEBERG_ASSIGN_OR_RAISE(auto* evaluator, - GetManifestEvaluator(schema, specs_by_id, spec_id)); - return evaluator->Evaluate(manifest); +Status ManifestFilterManager::DeleteFile(std::string_view path) { + ICEBERG_RETURN_UNEXPECTED(InvalidateFilteredCache()); + all_deletes_reference_manifests_ = false; + delete_paths_.insert(std::string(path)); + return {}; } -Result ManifestFilterManager::CanContainDeletedFiles( - const ManifestFile& manifest, const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, bool trust_manifest_references) { - // A manifest with no live files cannot contain files to delete. - // Missing counts mean the count is unknown; treat it as possibly non-zero. - bool has_live = !manifest.added_files_count.has_value() || - manifest.added_files_count.value() > 0 || - !manifest.existing_files_count.has_value() || - manifest.existing_files_count.value() > 0; - if (!has_live) return false; - - if (trust_manifest_references) { - // TODO(Guotao): Return whether this manifest is in the referenced manifest set. - return true; - } - - ICEBERG_ASSIGN_OR_RAISE(auto can_contain_dropped_files, - CanContainDroppedFiles(manifest)); - if (can_contain_dropped_files) return true; - - ICEBERG_ASSIGN_OR_RAISE(auto can_contain_expression_deletes, - CanContainExpressionDeletes(manifest, schema, specs_by_id)); - if (can_contain_expression_deletes) return true; - - return CanContainDroppedPartitions(manifest); +bool ManifestFilterManager::ContainsDeletes() const { + return !delete_paths_.empty() || !data_files_.empty() || !delete_files_.empty() || + delete_expression_->op() != Expression::Operation::kFalse || + !drop_partitions_.empty(); } -Result ManifestFilterManager::GetManifestEvaluator( - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, - int32_t spec_id) { - auto& evaluator = manifest_evaluator_cache_[spec_id]; - if (!evaluator) { - ICEBERG_ASSIGN_OR_RAISE(auto spec, PartitionSpecById(specs_by_id, spec_id)); - ICEBERG_ASSIGN_OR_RAISE(evaluator, ManifestEvaluator::MakeRowFilter( - delete_expr_, spec, *schema, case_sensitive_)); - } - return evaluator.get(); +Result> ManifestFilterManager::FilterManifests( + const TableMetadata& metadata, const std::shared_ptr& base_snapshot, + const ManifestWriterFactory& writer_factory) { + ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); + return FilterManifests(schema, metadata, base_snapshot, writer_factory); } -Result ManifestFilterManager::GetResidualEvaluator( - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, - int32_t spec_id) { - auto& evaluator = residual_evaluator_cache_[spec_id]; - if (!evaluator) { - ICEBERG_ASSIGN_OR_RAISE(auto spec, PartitionSpecById(specs_by_id, spec_id)); - ICEBERG_ASSIGN_OR_RAISE(evaluator, ResidualEvaluator::Make(delete_expr_, *spec, - *schema, case_sensitive_)); +Result> ManifestFilterManager::FilterManifests( + const std::shared_ptr& schema, const TableMetadata& metadata, + const std::shared_ptr& base_snapshot, + const ManifestWriterFactory& writer_factory) { + if (!base_snapshot) { + CollectDeletedFiles({}); + ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); + return std::vector{}; } - return evaluator.get(); -} - -Result ManifestFilterManager::ShouldDelete(const ManifestEntry& entry, - const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - int32_t manifest_spec_id) { - if (!entry.data_file) return false; - const DataFile& file = *entry.data_file; - int32_t spec_id = file.partition_spec_id.value_or(manifest_spec_id); - - // All delete branches share fail-any-delete handling. - auto marked_for_delete = [&]() -> Result { - if (fail_any_delete_) { - ICEBERG_ASSIGN_OR_RAISE(auto partition_path, - FormatPartitionPath(specs_by_id, file, spec_id)); - return ValidationFailed("Operation would delete existing data: {}", partition_path); - } - return true; - }; - - // Path/object-based and partition-drop checks. - bool object_delete = manifest_content_ == ManifestContent::kData - ? data_files_to_delete_.contains(file) - : delete_files_to_delete_.contains(file); - if (delete_paths_.count(file.file_path) || object_delete || - drop_partitions_.contains(spec_id, file.partition)) { - return marked_for_delete(); + ICEBERG_ASSIGN_OR_RAISE( + auto reader, ManifestListReader::Make(base_snapshot->manifest_list, file_io_)); + ICEBERG_ASSIGN_OR_RAISE(auto all_manifests, reader->Files()); + std::vector manifests; + for (const auto& manifest : all_manifests) { + if (manifest.content == manifest_content_) manifests.push_back(&manifest); } + TableMetadataCache metadata_cache(&metadata); + ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); + return FilterManifests(schema, specs_by_id.get(), manifests, writer_factory); +} - // Delete-manifest-specific cleanup (only for ManifestContent::kDeletes). - if (manifest_content_ == ManifestContent::kDeletes) { - // Drop delete files whose data sequence number is older than the minimum - // retained by the table (they can no longer match any live data rows). - // seq == 0 (kInitialSequenceNumber / nullopt) is intentionally excluded: - // those entries predate sequence number assignment and must not be pruned. - int64_t seq = entry.sequence_number.value_or(0); - if (min_sequence_number_ > 0 && seq > 0 && seq < min_sequence_number_) { - return marked_for_delete(); - } - - // Drop DVs that reference a data file that has been removed (dangling DV). - if (!removed_data_file_paths_.empty() && file.IsDeletionVector() && - file.referenced_data_file.has_value() && - removed_data_file_paths_.count(*file.referenced_data_file)) { - return marked_for_delete(); - } +Result> ManifestFilterManager::FilterManifests( + const std::shared_ptr& table_schema, const PartitionSpecsById& specs_by_id, + const std::vector& input_manifests, + const ManifestWriterFactory& writer_factory) { + std::vector manifests; + for (const auto* manifest : input_manifests) { + ICEBERG_PRECHECK(manifest != nullptr, "Cannot filter manifests: manifest is null"); + if (manifest->content == manifest_content_) manifests.push_back(manifest); } - - if (HasRowFilterExpression(delete_expr_)) { - ICEBERG_ASSIGN_OR_RAISE(auto* residual_eval, - GetResidualEvaluator(schema, specs_by_id, spec_id)); - ICEBERG_ASSIGN_OR_RAISE(auto residual_expr, - residual_eval->ResidualFor(file.partition)); - // TODO(Guotao): Cache strict/inclusive metrics evaluators per partition residual. - ICEBERG_ASSIGN_OR_RAISE( - auto strict_eval, - StrictMetricsEvaluator::Make(residual_expr, schema, case_sensitive_)); - ICEBERG_ASSIGN_OR_RAISE(auto strict_match, strict_eval->Evaluate(file)); - if (strict_match) { - return marked_for_delete(); - } - - ICEBERG_ASSIGN_OR_RAISE(auto incl_eval, InclusiveMetricsEvaluator::Make( - residual_expr, *schema, case_sensitive_)); - ICEBERG_ASSIGN_OR_RAISE(auto incl_match, incl_eval->Evaluate(file)); - if (incl_match) { - if (manifest_content_ == ManifestContent::kDeletes) { - return false; + if (manifests.empty()) { + CollectDeletedFiles({}); + ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); + return std::vector{}; + } + ICEBERG_PRECHECK(table_schema != nullptr, "Cannot filter manifests: schema is null"); + bool trust_manifest_references = CanTrustManifestReferences(manifests); + std::vector filtered(manifests.size()); + // Open all manifest files in parallel, using the index to avoid reordering. + TaskGroup tasks; + std::atomic failed{false}; + if (worker_pool_supplier_) tasks.SetExecutor(worker_pool_supplier_()); + for (size_t index = 0; index < manifests.size(); ++index) { + tasks.Submit([&, index]() -> Status { + if (failed.load()) return {}; + auto manifest = FilterManifest(table_schema, specs_by_id, *manifests[index], + trust_manifest_references, writer_factory); + if (!manifest) { + failed.store(true); + return ::iceberg::unexpected(std::move(manifest.error())); } - return ValidationFailed( - "Cannot delete file where some, but not all, rows match filter: {}", - file.file_path); - } + filtered[index] = std::move(manifest.value()); + return {}; + }); } - - return false; + ICEBERG_RETURN_UNEXPECTED(std::move(tasks).Run()); + CollectDeletedFiles(filtered); + for (const auto& file : deleted_files_) { + InsertFile(manifest_content_, data_files_, delete_files_, file); + } + ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); + return filtered; } bool ManifestFilterManager::CanTrustManifestReferences( - const std::vector&) const { - // TODO(Guotao): add DataFile manifest locations and use them to skip unrelated - // manifests. Until then, take the conservative path. - return false; + const std::vector& manifests) const { + std::unordered_set manifest_locations; + for (const auto* manifest : manifests) + manifest_locations.insert(manifest->manifest_path); + return all_deletes_reference_manifests_ && !manifests_with_deletes_.empty() && + std::ranges::all_of(manifests_with_deletes_, [&](const auto& location) { + return manifest_locations.contains(location); + }); } -Result ManifestFilterManager::FilterManifest( - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, - const ManifestFile& manifest, bool trust_manifest_references, - const ManifestWriterFactory& writer_factory) { - auto cached = filtered_manifests_.find(manifest); - if (cached != filtered_manifests_.end()) { - auto deleted_iter = filtered_manifest_to_deleted_files_.find(cached->second); - if (deleted_iter != filtered_manifest_to_deleted_files_.end()) { - for (const auto& file : deleted_iter->second.files) { - AddDeletedFileToManager(manifest_content_, data_files_, delete_files_, - deleted_files_, deleted_data_file_set_, - deleted_delete_file_set_, file); +Result ManifestFilterManager::BuildSummary( + const std::vector& manifests, + const PartitionSpecsById& specs_by_id) const { + SnapshotSummaryBuilder summary_builder; + for (const auto& manifest : manifests) { + auto result = filtered_manifest_results_.find(manifest); + if (result != filtered_manifest_results_.end()) { + ICEBERG_ASSIGN_OR_RAISE(auto manifest_spec, + PartitionSpecById(specs_by_id, manifest.partition_spec_id)); + for (const auto& file : result->second.files) { + ICEBERG_RETURN_UNEXPECTED(summary_builder.DeletedFile(*manifest_spec, *file)); } - duplicate_deletes_count_ += deleted_iter->second.duplicate_deletes_count; + summary_builder.IncrementDuplicateDeletes(result->second.duplicate_delete_count); } - return cached->second; } - - ICEBERG_ASSIGN_OR_RAISE( - auto can_contain_deleted_files, - CanContainDeletedFiles(manifest, schema, specs_by_id, trust_manifest_references)); - if (!can_contain_deleted_files) { - filtered_manifests_.emplace(manifest, manifest); - return manifest; - } - - int32_t spec_id = manifest.partition_spec_id; - ICEBERG_ASSIGN_OR_RAISE(auto spec, PartitionSpecById(specs_by_id, spec_id)); - ICEBERG_ASSIGN_OR_RAISE(auto reader, - ManifestReader::Make(manifest, file_io_, schema, spec)); - ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntries()); - - ICEBERG_ASSIGN_OR_RAISE(auto has_deleted_files, - ManifestHasDeletedFiles(entries, schema, specs_by_id, spec_id)); - if (!has_deleted_files) { - filtered_manifests_.emplace(manifest, manifest); - return manifest; - } - - ICEBERG_ASSIGN_OR_RAISE(auto filtered_manifest, - FilterManifestWithDeletedFiles(entries, spec_id, schema, - specs_by_id, writer_factory)); - filtered_manifests_.emplace(manifest, filtered_manifest); - ++replaced_manifests_count_; - return filtered_manifest; + return summary_builder; } -Result ManifestFilterManager::ManifestHasDeletedFiles( - const std::vector& entries, const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, int32_t manifest_spec_id) { - for (const auto& entry : entries) { - ICEBERG_ASSIGN_OR_RAISE(auto should_delete, - ShouldDelete(entry, schema, specs_by_id, manifest_spec_id)); - if (should_delete) { - return true; +Status ManifestFilterManager::ValidateRequiredDeletes() const { + if (fail_missing_delete_paths_) { + std::string missing_files; + auto append_missing = [&missing_files](const std::string& path) { + if (!missing_files.empty()) missing_files += ","; + missing_files += path; + }; + for (const auto& file : data_files_) { + if (!deleted_data_file_set_.contains(file)) append_missing(file->file_path); + } + for (const auto& file : delete_files_) { + if (!deleted_delete_file_set_.contains(file)) append_missing(file->file_path); + } + if (!missing_files.empty()) { + return ValidationFailed("Missing required files to delete: {}", missing_files); + } + std::unordered_set deleted_file_paths; + for (const auto& file : deleted_files_) deleted_file_paths.insert(file->file_path); + for (const auto& path : delete_paths_) { + if (!deleted_file_paths.contains(path)) append_missing(path); + } + if (!missing_files.empty()) { + return ValidationFailed("Missing required files to delete: {}", missing_files); } } - return false; + return {}; } -Result ManifestFilterManager::FilterManifestWithDeletedFiles( - const std::vector& entries, int32_t manifest_spec_id, - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, - const ManifestWriterFactory& writer_factory) { - ICEBERG_ASSIGN_OR_RAISE(auto writer, - writer_factory(manifest_spec_id, manifest_content_)); - std::vector> deleted_files; - DataFileSet deleted_data_file_set; - DeleteFileSet deleted_file_set; - int32_t duplicate_deletes_count = 0; - for (const auto& entry : entries) { - ICEBERG_ASSIGN_OR_RAISE(auto should_delete, - ShouldDelete(entry, schema, specs_by_id, manifest_spec_id)); - if (should_delete) { - if (entry.data_file) { - auto file = std::make_shared(*entry.data_file); - AddDeletedFileToManager(manifest_content_, data_files_, delete_files_, - deleted_files_, deleted_data_file_set_, - deleted_delete_file_set_, file); - if (!AddDeletedFileToManifest(manifest_content_, deleted_files, - deleted_data_file_set, deleted_file_set, file)) { - ++duplicate_deletes_count; +void ManifestFilterManager::CollectDeletedFiles( + const std::vector& manifests) { + deleted_files_.clear(); + deleted_data_file_set_.clear(); + deleted_delete_file_set_.clear(); + duplicate_deletes_count_ = 0; + for (const auto& manifest : manifests) { + auto result = filtered_manifest_results_.find(manifest); + if (result != filtered_manifest_results_.end()) { + for (const auto& file : result->second.files) { + if (InsertFile(manifest_content_, deleted_data_file_set_, + deleted_delete_file_set_, file)) { + deleted_files_.push_back(file); } } - ICEBERG_RETURN_UNEXPECTED(writer->WriteDeletedEntry(entry)); - } else { - ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); + duplicate_deletes_count_ += result->second.duplicate_delete_count; } } +} - ICEBERG_RETURN_UNEXPECTED(writer->Close()); - ICEBERG_ASSIGN_OR_RAISE(auto filtered_manifest, writer->ToManifestFile()); - duplicate_deletes_count_ += duplicate_deletes_count; - filtered_manifest_to_deleted_files_[filtered_manifest] = FilteredManifestDeletes{ - .files = std::move(deleted_files), - .duplicate_deletes_count = duplicate_deletes_count, - }; - return filtered_manifest; +const std::vector>& ManifestFilterManager::DeletedFiles() + const { + return deleted_files_; +} + +Status ManifestFilterManager::CleanUncommitted( + const std::unordered_set& committed) { + // Iterate over a copy of entries to avoid concurrent modification. + auto filter_entries = std::vector>{ + filtered_manifests_.begin(), filtered_manifests_.end()}; + for (const auto& [manifest, filtered] : filter_entries) { + if (!committed.contains(filtered.manifest_path)) { + if (manifest != filtered) { + std::ignore = delete_file_(filtered.manifest_path); + --replaced_manifests_count_; + } + filtered_manifests_.erase(manifest); + filtered_manifest_results_.erase(filtered); + } + } + return {}; } Status ManifestFilterManager::InvalidateFilteredCache() { @@ -501,164 +453,218 @@ Status ManifestFilterManager::InvalidateFilteredCache() { return {}; } -void ManifestFilterManager::ResetDeletedFiles() { - data_files_.clear(); - for (const auto& file : data_files_to_delete_) { - data_files_.insert(file); +Result ManifestFilterManager::FilterManifest( + const std::shared_ptr& table_schema, const PartitionSpecsById& specs_by_id, + const ManifestFile& manifest, bool trust_manifest_references, + const ManifestWriterFactory& writer_factory) { + { + std::lock_guard lock(cache_mutex_); + auto cached = filtered_manifests_.find(manifest); + if (cached != filtered_manifests_.end()) return cached->second; + } + ICEBERG_ASSIGN_OR_RAISE(auto can_contain_deleted_files, + CanContainDeletedFiles(manifest, table_schema, specs_by_id, + trust_manifest_references)); + if (!can_contain_deleted_files) { + std::lock_guard lock(cache_mutex_); + filtered_manifests_[manifest] = manifest; + return manifest; } - delete_files_.clear(); - for (const auto& file : delete_files_to_delete_) { - delete_files_.insert(file); + ICEBERG_ASSIGN_OR_RAISE(auto spec, + PartitionSpecById(specs_by_id, manifest.partition_spec_id)); + ICEBERG_ASSIGN_OR_RAISE(auto reader, + ManifestReader::Make(manifest, file_io_, table_schema, spec)); + ICEBERG_ASSIGN_OR_RAISE(auto evaluator, + PartitionAndMetricsEvaluator::Make( + table_schema, *spec, delete_expression_, case_sensitive_)); + // The C++ reader materializes live entries; both passes reuse that vector. + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->LiveEntries()); + ICEBERG_ASSIGN_OR_RAISE(auto has_deleted_files, + ManifestHasDeletedFiles(*evaluator, manifest, *spec, entries)); + if (has_deleted_files) { + ICEBERG_ASSIGN_OR_RAISE( + auto filtered, + FilterManifestWithDeletedFiles(*evaluator, manifest, entries, writer_factory)); + ++replaced_manifests_count_; + return filtered; } + std::lock_guard lock(cache_mutex_); + filtered_manifests_[manifest] = manifest; + return manifest; } -Status ManifestFilterManager::ValidateRequiredDeletes() const { - if (!fail_missing_delete_paths_) { - return {}; - } - - std::string missing_files; - const auto append_missing = [&missing_files](const std::string& path) { - if (!missing_files.empty()) missing_files += ","; - missing_files += path; - }; - for (const auto& key : data_files_to_delete_) { - if (!deleted_data_file_set_.contains(key)) { - append_missing(key->file_path); - } - } - for (const auto& key : delete_files_to_delete_) { - if (!deleted_delete_file_set_.contains(key)) { - append_missing(key->file_path); - } - } - if (!missing_files.empty()) { - return ValidationFailed("Missing required files to delete: {}", missing_files); - } - - std::string missing_paths; - for (const auto& path : delete_paths_) { - bool found = false; - for (const auto& deleted_file : deleted_files_) { - if (deleted_file != nullptr && deleted_file->file_path == path) { - found = true; - break; - } - } - if (!found) { - if (!missing_paths.empty()) missing_paths += ","; - missing_paths += path; - } - } - if (!missing_paths.empty()) { - return ValidationFailed("Missing required files to delete: {}", missing_paths); - } - return {}; +Result ManifestFilterManager::CanContainDeletedFiles( + const ManifestFile& manifest, const std::shared_ptr& table_schema, + const PartitionSpecsById& specs_by_id, bool trust_manifest_references) const { + if (HasNoLiveFiles(manifest)) return false; + if (trust_manifest_references) + return manifests_with_deletes_.contains(manifest.manifest_path); + ICEBERG_ASSIGN_OR_RAISE(auto dropped_files, + CanContainDroppedFiles(manifest, *table_schema, specs_by_id)); + if (dropped_files) return true; + ICEBERG_ASSIGN_OR_RAISE( + auto expression_deletes, + CanContainExpressionDeletes(manifest, table_schema, specs_by_id)); + if (expression_deletes) return true; + return CanContainDroppedPartitions(manifest, *table_schema, specs_by_id); } -Result> ManifestFilterManager::FilterManifests( - const TableMetadata& metadata, const std::shared_ptr& base_snapshot, - const ManifestWriterFactory& writer_factory) { - ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); - return FilterManifests(schema, metadata, base_snapshot, writer_factory); +bool ManifestFilterManager::HasNoLiveFiles(const ManifestFile& manifest) { + return !manifest.has_added_files() && !manifest.has_existing_files(); } -Result> ManifestFilterManager::FilterManifests( - const std::shared_ptr& schema, const TableMetadata& metadata, - const std::shared_ptr& base_snapshot, - const ManifestWriterFactory& writer_factory) { - ResetDeletedFiles(); - deleted_files_.clear(); - deleted_data_file_set_.clear(); - deleted_delete_file_set_.clear(); - duplicate_deletes_count_ = 0; - if (!base_snapshot) { - ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); - return std::vector{}; +Result ManifestFilterManager::CanContainExpressionDeletes( + const ManifestFile& manifest, const std::shared_ptr& table_schema, + const PartitionSpecsById& specs_by_id) const { + if (delete_expression_ && delete_expression_->op() != Expression::Operation::kFalse) { + ICEBERG_ASSIGN_OR_RAISE(auto spec, + PartitionSpecById(specs_by_id, manifest.partition_spec_id)); + ICEBERG_ASSIGN_OR_RAISE(auto manifest_evaluator, ManifestEvaluator::MakeRowFilter( + delete_expression_, spec, + *table_schema, case_sensitive_)); + return manifest_evaluator->Evaluate(manifest); } + return false; +} - ICEBERG_PRECHECK(file_io_ != nullptr, "Cannot filter manifests: FileIO is null"); - - ICEBERG_ASSIGN_OR_RAISE( - auto list_reader, ManifestListReader::Make(base_snapshot->manifest_list, file_io_)); - ICEBERG_ASSIGN_OR_RAISE(auto all_manifests, list_reader->Files()); +Result ManifestFilterManager::CanContainDroppedPartitions( + const ManifestFile& manifest, const Schema& table_schema, + const PartitionSpecsById& specs_by_id) const { + if (!drop_partitions_.empty()) + return CanContainAny(manifest, drop_partitions_, table_schema, specs_by_id); + return false; +} - std::vector manifests; - manifests.reserve(all_manifests.size()); - for (auto& manifest : all_manifests) { - manifests.push_back(&manifest); +Result ManifestFilterManager::CanContainDroppedFiles( + const ManifestFile& manifest, const Schema& table_schema, + const PartitionSpecsById& specs_by_id) const { + if (!delete_paths_.empty()) { + return true; + } else if (!data_files_.empty() || !delete_files_.empty()) { + return CanContainAny(manifest, delete_file_partitions_, table_schema, specs_by_id); + } else if (!removed_data_file_paths_.empty()) { + return true; } - - TableMetadataCache metadata_cache(&metadata); - ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); - - return FilterManifests(schema, specs_by_id.get(), manifests, writer_factory); + return false; } -Result> ManifestFilterManager::FilterManifests( - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, - const std::vector& input_manifests, - const ManifestWriterFactory& writer_factory) { - ICEBERG_PRECHECK(schema != nullptr, "Cannot filter manifests: schema is null"); - ICEBERG_PRECHECK(file_io_ != nullptr, "Cannot filter manifests: FileIO is null"); - - std::vector manifests; - manifests.reserve(input_manifests.size()); - for (const auto* manifest : input_manifests) { - ICEBERG_PRECHECK(manifest != nullptr, "Cannot filter manifests: manifest is null"); - if (manifest->content == manifest_content_) { - manifests.push_back(manifest); +Result ManifestFilterManager::ManifestHasDeletedFiles( + PartitionAndMetricsEvaluator& evaluator, const ManifestFile& manifest, + const PartitionSpec& spec, const std::vector& entries) const { + if (manifests_with_deletes_.contains(manifest.manifest_path)) return true; + bool is_delete = manifest_content_ == ManifestContent::kDeletes; + for (const auto& entry : entries) { + const auto& file = *entry.data_file; + bool marked_for_delete = + delete_paths_.contains(file.file_path) || + (is_delete ? delete_files_.contains(file) : data_files_.contains(file)) || + drop_partitions_.contains( + file.partition_spec_id.value_or(manifest.partition_spec_id), + file.partition) || + (is_delete && entry.status != ManifestStatus::kDeleted && + entry.sequence_number.value_or(0) > 0 && + entry.sequence_number.value_or(0) < min_sequence_number_) || + (is_delete && IsDanglingDV(file)); + bool rows_might_match = marked_for_delete; + if (!marked_for_delete) { + ICEBERG_ASSIGN_OR_RAISE(rows_might_match, evaluator.RowsMightMatch(file)); + } + if (rows_might_match) { + bool all_rows_match = marked_for_delete; + if (!marked_for_delete) { + ICEBERG_ASSIGN_OR_RAISE(all_rows_match, evaluator.RowsMustMatch(file)); + } + if (!all_rows_match && !is_delete) { + return ValidationFailed( + "Cannot delete file where some, but not all, rows match filter {}: {}", + delete_expression_->ToString(), file.file_path); + } + if (all_rows_match) { + if (fail_any_delete_) { + ICEBERG_ASSIGN_OR_RAISE(auto partition_path, + spec.PartitionPath(file.partition)); + return ValidationFailed("Operation would delete existing data: {}", + partition_path); + } + return true; + } } } - - ResetDeletedFiles(); - deleted_files_.clear(); - deleted_data_file_set_.clear(); - deleted_delete_file_set_.clear(); - duplicate_deletes_count_ = 0; - if (manifests.empty()) { - ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); - return std::vector{}; - } - - bool trust_manifest_references = CanTrustManifestReferences(manifests); - manifest_evaluator_cache_.clear(); - residual_evaluator_cache_.clear(); - - // TODO(Guotao): Parallelize manifest filtering with per-manifest results, then - // merge found paths and deleted files after the loop. - std::vector filtered; - filtered.reserve(manifests.size()); - for (const auto* manifest_ptr : manifests) { - ICEBERG_ASSIGN_OR_RAISE(auto filtered_manifest, - FilterManifest(schema, specs_by_id, *manifest_ptr, - trust_manifest_references, writer_factory)); - filtered.push_back(std::move(filtered_manifest)); - } - - ICEBERG_RETURN_UNEXPECTED(ValidateRequiredDeletes()); - return filtered; + return false; } -Status ManifestFilterManager::CleanUncommitted( - const std::unordered_set& committed) { - auto entries = std::vector>{ - filtered_manifests_.begin(), filtered_manifests_.end()}; - for (const auto& [manifest, filtered] : entries) { - if (committed.contains(filtered.manifest_path)) { - continue; - } +bool ManifestFilterManager::IsDanglingDV(const DataFile& file) const { + return file.IsDeletionVector() && file.referenced_data_file.has_value() && + removed_data_file_paths_.contains(*file.referenced_data_file); +} - if (manifest != filtered) { - std::ignore = delete_file_(filtered.manifest_path); - if (replaced_manifests_count_ > 0) { - --replaced_manifests_count_; +Result ManifestFilterManager::FilterManifestWithDeletedFiles( + PartitionAndMetricsEvaluator& evaluator, const ManifestFile& manifest, + const std::vector& entries, + const ManifestWriterFactory& writer_factory) { + bool is_delete = manifest_content_ == ManifestContent::kDeletes; + DataFileSet deleted_data_files; + DeleteFileSet deleted_delete_files; + FilteredManifestResult result; + ICEBERG_ASSIGN_OR_RAISE(auto writer, + writer_factory(manifest.partition_spec_id, manifest_content_)); + auto write_entries = [&]() -> Status { + for (const auto& entry : entries) { + const auto& file = *entry.data_file; + bool is_dangling_dv = is_delete && IsDanglingDV(file); + bool marked_for_delete = + is_dangling_dv || delete_paths_.contains(file.file_path) || + (is_delete ? delete_files_.contains(file) : data_files_.contains(file)) || + drop_partitions_.contains( + file.partition_spec_id.value_or(manifest.partition_spec_id), + file.partition) || + (is_delete && entry.status != ManifestStatus::kDeleted && + entry.sequence_number.value_or(0) > 0 && + entry.sequence_number.value_or(0) < min_sequence_number_); + bool rows_might_match = marked_for_delete; + if (!marked_for_delete) { + ICEBERG_ASSIGN_OR_RAISE(rows_might_match, evaluator.RowsMightMatch(file)); + } + if (rows_might_match) { + bool all_rows_match = marked_for_delete; + if (!marked_for_delete) { + ICEBERG_ASSIGN_OR_RAISE(all_rows_match, evaluator.RowsMustMatch(file)); + } + if (!all_rows_match && !is_delete) { + return ValidationFailed( + "Cannot delete file where some, but not all, rows match filter {}: {}", + delete_expression_->ToString(), file.file_path); + } + if (all_rows_match) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteDeletedEntry(entry)); + auto file_copy = file.CopyWithoutStats(); + if (!InsertFile(manifest_content_, deleted_data_files, deleted_delete_files, + file_copy)) { + ICEBERG_LOG_WARN("Deleting a duplicate path from manifest {}: {}", + manifest.manifest_path, file.file_path); + ++result.duplicate_delete_count; + } else { + // Only add a new delete to keep the snapshot summary accurate for duplicates. + result.files.push_back(std::move(file_copy)); + } + } else { + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); + } + } else { + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); } } - filtered_manifests_.erase(manifest); - filtered_manifest_to_deleted_files_.erase(filtered); - } - return {}; + return {}; + }; + auto write_status = write_entries(); + // Java closes in finally: a close failure takes precedence over a write failure. + ICEBERG_RETURN_UNEXPECTED(writer->Close()); + ICEBERG_RETURN_UNEXPECTED(write_status); + ICEBERG_ASSIGN_OR_RAISE(auto filtered, writer->ToManifestFile()); + std::lock_guard lock(cache_mutex_); + filtered_manifests_[manifest] = filtered; + filtered_manifest_results_[filtered] = std::move(result); + return filtered; } } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_filter_manager.h b/src/iceberg/manifest/manifest_filter_manager.h index e742812cc2..c5644d022d 100644 --- a/src/iceberg/manifest/manifest_filter_manager.h +++ b/src/iceberg/manifest/manifest_filter_manager.h @@ -23,9 +23,11 @@ /// Filters an existing snapshot's manifest list, marking data files as DELETED /// or EXISTING based on row-filter expressions, exact path deletes, and partition drops. +#include #include #include #include +#include #include #include #include @@ -38,6 +40,7 @@ #include "iceberg/snapshot.h" #include "iceberg/type_fwd.h" #include "iceberg/util/data_file_set.h" +#include "iceberg/util/executor.h" #include "iceberg/util/partition_value_util.h" namespace iceberg { @@ -46,20 +49,22 @@ namespace iceberg { /// /// The manager accumulates delete conditions incrementally, then applies them all /// at once in a single FilterManifests() call. Manifests that contain no deleted -/// entries are returned unchanged (no I/O). Manifests that do contain deleted +/// entries are returned unchanged. Manifests that do contain deleted /// entries are rewritten with those entries marked DELETED. /// -/// TODO(Guotao): For ManifestContent::kDeletes, implement cleanup for orphan delete files -/// and dangling deletion vectors. -/// -/// \note This class is non-copyable and non-movable. +/// \note This class is non-copyable and non-movable. Public calls must not overlap. +/// A configured worker pool runs manifest reads and writer callbacks concurrently; +/// those callbacks and their shared state must support concurrent calls. class ICEBERG_EXPORT ManifestFilterManager { public: using PartitionSpecsById = std::unordered_map>; + using WorkerPoolSupplier = std::function; + static Result> Make( ManifestContent content, std::shared_ptr file_io, - std::function delete_file = {}); + std::function delete_file = {}, + WorkerPoolSupplier worker_pool_supplier = {}); ~ManifestFilterManager(); ManifestFilterManager(const ManifestFilterManager&) = delete; @@ -67,8 +72,7 @@ class ICEBERG_EXPORT ManifestFilterManager { /// \brief Register a row-filter expression. /// - /// Any manifest entry whose column metrics indicate the file may satisfy the - /// expression will be marked DELETED. + /// A file is deleted when all of its rows match this or another registered filter. /// /// \param expr The expression to match files against Status DeleteByRowFilter(std::shared_ptr expr); @@ -85,18 +89,20 @@ class ICEBERG_EXPORT ManifestFilterManager { /// \brief Register a file object for deletion. /// - /// Any manifest entry whose file_path matches file->file_path will be marked - /// DELETED. Duplicate registrations (same path) are silently ignored. + /// Data files match by path; delete files match by path, content offset and size. /// /// \param file The data/delete file to delete (must not be null) Status DeleteFile(std::shared_ptr file); - /// \brief Returns the set of file objects marked for deletion by this manager. + /// \brief Returns cumulative data-file objects marked for deletion. /// /// Includes file objects explicitly registered for deletion plus files deleted while - /// filtering manifests. + /// filtering manifests. For delete-file managers use DeleteFilesToBeDeleted(). const DataFileSet& FilesToBeDeleted() const; + /// \brief Returns cumulative delete-file objects, preserving deletion-vector identity. + const DeleteFileSet& DeleteFilesToBeDeleted() const; + /// \brief Returns content-file objects deleted by the most recent /// FilterManifests() call, deduplicated by content-file identity. const std::vector>& DeletedFiles() const; @@ -127,10 +133,10 @@ class ICEBERG_EXPORT ManifestFilterManager { /// manifest entry matches a delete condition. void FailAnyDelete(); - /// \brief Returns the number of manifests rewritten (replaced) by the last - /// FilterManifests() call. A manifest is replaced when it contained deleted entries - /// and was rewritten with those entries marked DELETED. - int32_t ReplacedManifestsCount() const { return replaced_manifests_count_; } + /// \brief Returns the number of manifests replaced by cached filtered outputs. + /// Retries reuse these outputs without incrementing the count; cleanup removes + /// the corresponding replacements from the count. + int32_t ReplacedManifestsCount() const { return replaced_manifests_count_.load(); } /// \brief Returns true if any delete condition has been registered. bool ContainsDeletes() const; @@ -199,115 +205,84 @@ class ICEBERG_EXPORT ManifestFilterManager { Status CleanUncommitted(const std::unordered_set& committed); private: - ManifestFilterManager(ManifestContent content, std::shared_ptr file_io, - std::function delete_file); - - /// \brief Returns true if the manifest might contain files matching any expression. - Result CanContainExpressionDeletes(const ManifestFile& manifest, - const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id); + class PartitionAndMetricsEvaluator; - /// \brief Returns true if the manifest might contain files in a dropped partition. - /// - /// Checks whether the manifest's partition_spec_id matches any spec_id registered - /// via DropPartition(). Manifests from a different spec cannot contain the dropped - /// partition values. - Result CanContainDroppedPartitions(const ManifestFile& manifest) const; - - /// \brief Returns true if the manifest might contain path-deleted files. - Result CanContainDroppedFiles(const ManifestFile& manifest) const; - - /// \brief Returns true if the manifest possibly contains any deleted file. - Result CanContainDeletedFiles(const ManifestFile& manifest, - const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - bool trust_manifest_references); + ManifestFilterManager(ManifestContent content, std::shared_ptr file_io, + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier); bool CanTrustManifestReferences( const std::vector& manifests) const; + Status ValidateRequiredDeletes() const; + void CollectDeletedFiles(const std::vector& manifests); + Status InvalidateFilteredCache(); - struct FilteredManifestDeletes { - std::vector> files; - int32_t duplicate_deletes_count = 0; - }; - - Result FilterManifest(const std::shared_ptr& schema, + Result FilterManifest(const std::shared_ptr& table_schema, const PartitionSpecsById& specs_by_id, const ManifestFile& manifest, bool trust_manifest_references, const ManifestWriterFactory& writer_factory); - - Result ManifestHasDeletedFiles(const std::vector& entries, - const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - int32_t manifest_spec_id); - + Result CanContainDeletedFiles(const ManifestFile& manifest, + const std::shared_ptr& table_schema, + const PartitionSpecsById& specs_by_id, + bool trust_manifest_references) const; + static bool HasNoLiveFiles(const ManifestFile& manifest); + Result CanContainExpressionDeletes(const ManifestFile& manifest, + const std::shared_ptr& table_schema, + const PartitionSpecsById& specs_by_id) const; + Result CanContainDroppedPartitions(const ManifestFile& manifest, + const Schema& table_schema, + const PartitionSpecsById& specs_by_id) const; + Result CanContainDroppedFiles(const ManifestFile& manifest, + const Schema& table_schema, + const PartitionSpecsById& specs_by_id) const; + Result ManifestHasDeletedFiles(PartitionAndMetricsEvaluator& evaluator, + const ManifestFile& manifest, + const PartitionSpec& spec, + const std::vector& entries) const; + bool IsDanglingDV(const DataFile& file) const; Result FilterManifestWithDeletedFiles( - const std::vector& entries, int32_t manifest_spec_id, - const std::shared_ptr& schema, const PartitionSpecsById& specs_by_id, + PartitionAndMetricsEvaluator& evaluator, const ManifestFile& manifest, + const std::vector& entries, const ManifestWriterFactory& writer_factory); - Status ValidateRequiredDeletes() const; - - Status InvalidateFilteredCache(); - void ResetDeletedFiles(); - - /// \brief Get or create a ManifestEvaluator for the given spec. - Result GetManifestEvaluator(const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - int32_t spec_id); - - /// \brief Get or create a ResidualEvaluator for the given spec. - Result GetResidualEvaluator(const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - int32_t spec_id); - - /// \brief Check whether a single entry should be deleted. - Result ShouldDelete(const ManifestEntry& entry, - const std::shared_ptr& schema, - const PartitionSpecsById& specs_by_id, - int32_t manifest_spec_id); + struct FilteredManifestResult { + std::vector> files; + int32_t duplicate_delete_count = 0; + }; const ManifestContent manifest_content_; std::shared_ptr file_io_; std::function delete_file_; - - std::shared_ptr delete_expr_; - std::unordered_set delete_paths_; - // Delete files explicitly registered for deletion by object identity. - DeleteFileSet delete_files_to_delete_; - // Data files explicitly registered for deletion by object identity. - DataFileSet data_files_to_delete_; - // Data files to remove: explicit object deletes plus files found while filtering. + PartitionSet delete_file_partitions_; + // Java Set uses distinct data-file and delete-file identity semantics. DataFileSet data_files_; - // Delete files to remove: explicit object deletes plus files found while filtering. DeleteFileSet delete_files_; - std::unordered_map - filtered_manifest_to_deleted_files_; - // Ordered files deleted by the latest filter pass, used for summaries. - std::vector> deleted_files_; - // Data-file identity set for latest-pass dedup and required-delete validation. - DataFileSet deleted_data_file_set_; - // Delete-file identity set for latest-pass dedup and required-delete validation. - DeleteFileSet deleted_delete_file_set_; + std::unordered_set manifests_with_deletes_; PartitionSet drop_partitions_; - bool fail_missing_delete_paths_{false}; - bool fail_any_delete_{false}; - bool case_sensitive_{true}; - int32_t duplicate_deletes_count_{0}; - int32_t replaced_manifests_count_{0}; + std::unordered_set delete_paths_; + std::atomic replaced_manifests_count_{0}; - // minimum data sequence number; delete entries older than this are dropped + std::shared_ptr delete_expression_; int64_t min_sequence_number_{0}; - // paths of data files that were removed; DVs referencing these are dangling + bool fail_any_delete_{false}; + bool fail_missing_delete_paths_{false}; + bool case_sensitive_{true}; + bool all_deletes_reference_manifests_{true}; std::unordered_set removed_data_file_paths_; + // Cache filtered manifests to avoid extra work when commits fail. std::unordered_map filtered_manifests_; + // Track where files were deleted to validate retries quickly. + std::unordered_map filtered_manifest_results_; + mutable std::mutex cache_mutex_; + WorkerPoolSupplier worker_pool_supplier_; - std::unordered_map> - manifest_evaluator_cache_; - std::unordered_map> - residual_evaluator_cache_; + // Materialized latest-pass views for the C++ accessors and validation. + std::vector> deleted_files_; + DataFileSet deleted_data_file_set_; + DeleteFileSet deleted_delete_file_set_; + int32_t duplicate_deletes_count_{0}; }; } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_merge_manager.cc b/src/iceberg/manifest/manifest_merge_manager.cc index fc9183d13b..5d5f879e54 100644 --- a/src/iceberg/manifest/manifest_merge_manager.cc +++ b/src/iceberg/manifest/manifest_merge_manager.cc @@ -20,10 +20,7 @@ #include "iceberg/manifest/manifest_merge_manager.h" #include -#include -#include #include -#include #include #include #include @@ -34,31 +31,24 @@ #include "iceberg/manifest/manifest_reader.h" #include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" +#include "iceberg/util/task_group.h" namespace iceberg { -namespace { - -size_t CombineHash(size_t seed, size_t value) { - return seed ^ (value + 0x9e3779b9 + (seed << 6) + (seed >> 2)); -} - -} // namespace - ManifestMergeManager::ManifestMergeManager( ManifestContent content, int64_t target_size_bytes, int32_t min_count_to_merge, bool merge_enabled, std::shared_ptr file_io, SnapshotIdSupplier snapshot_id_supplier, - std::function delete_file) + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier) : manifest_content_(content), target_size_bytes_(target_size_bytes), min_count_to_merge_(min_count_to_merge), merge_enabled_(merge_enabled), file_io_(std::move(file_io)), snapshot_id_supplier_(std::move(snapshot_id_supplier)), - delete_file_(std::move(delete_file)) { - ICEBERG_DCHECK(file_io_, "FileIO cannot be null"); - ICEBERG_DCHECK(snapshot_id_supplier_, "Snapshot ID supplier cannot be null"); + delete_file_(std::move(delete_file)), + worker_pool_supplier_(std::move(worker_pool_supplier)) { if (delete_file_ == nullptr) { delete_file_ = [this](const std::string& location) { return file_io_->DeleteFile(location); @@ -70,234 +60,229 @@ Result> ManifestMergeManager::Make( ManifestContent content, int64_t target_size_bytes, int32_t min_count_to_merge, bool merge_enabled, std::shared_ptr file_io, SnapshotIdSupplier snapshot_id_supplier, - std::function delete_file) { + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier) { ICEBERG_PRECHECK(file_io != nullptr, "FileIO cannot be null"); ICEBERG_PRECHECK(snapshot_id_supplier != nullptr, "Snapshot ID supplier cannot be null"); return std::unique_ptr(new ManifestMergeManager( content, target_size_bytes, min_count_to_merge, merge_enabled, std::move(file_io), - std::move(snapshot_id_supplier), std::move(delete_file))); + std::move(snapshot_id_supplier), std::move(delete_file), + std::move(worker_pool_supplier))); } Result> ManifestMergeManager::MergeManifests( const std::vector& existing_manifests, const std::vector& new_manifests, const TableMetadata& metadata, const ManifestWriterFactory& writer_factory) { - auto append_manifest = [this](const ManifestFile& manifest, - std::vector& manifests) -> Status { - ICEBERG_PRECHECK(manifest.content == manifest_content_, - "Cannot merge manifest with unexpected content"); - manifests.push_back(&manifest); - return {}; - }; + std::vector manifests = new_manifests; + manifests.insert(manifests.end(), existing_manifests.begin(), existing_manifests.end()); + return MergeManifests(manifests, metadata, writer_factory); +} - std::vector all; - all.reserve(new_manifests.size() + existing_manifests.size()); - for (const auto& manifest : new_manifests) { - ICEBERG_RETURN_UNEXPECTED(append_manifest(manifest, all)); +Result> ManifestMergeManager::MergeManifests( + const std::vector& manifests, const TableMetadata& metadata, + const ManifestWriterFactory& writer_factory) { + if (!merge_enabled_ || manifests.empty()) { + return manifests; } - for (const auto& manifest : existing_manifests) { - ICEBERG_RETURN_UNEXPECTED(append_manifest(manifest, all)); + + // C++ uses a runtime content tag instead of Java's ContentFile type parameter. + for (const auto& manifest : manifests) { + ICEBERG_PRECHECK(manifest.content == manifest_content_, + "Cannot merge manifest with unexpected content"); } - if (all.empty() || !merge_enabled_) { - return all | - std::views::transform([](const ManifestFile* manifest) { return *manifest; }) | - std::ranges::to>(); + const ManifestFile& first = manifests.front(); + std::vector merged; + auto groups = GroupBySpec(manifests); + for (const auto& [spec_id, group] : groups) { + ICEBERG_ASSIGN_OR_RAISE(auto result, + MergeGroup(first, spec_id, group, metadata, writer_factory)); + std::ranges::move(result, std::back_inserter(merged)); } + return merged; +} - const auto* first = all.front(); - std::map, std::greater<>> by_spec; - std::ranges::for_each(all, [&by_spec](const ManifestFile* manifest) { - by_spec[manifest->partition_spec_id].push_back(manifest); - }); +Status ManifestMergeManager::CleanUncommitted( + const std::unordered_set& committed) { + // Iterate over a copy of entries to avoid concurrent modification. + auto entries = [this] { + std::lock_guard lock(merged_manifests_mutex_); + return merged_manifests_; + }(); + for (const auto& [bin, merged] : entries) { + // Delete any new merged manifests that aren't in the committed list. + if (!committed.contains(merged.manifest_path)) { + std::ignore = delete_file_(merged.manifest_path); + { + std::lock_guard lock(merged_manifests_mutex_); + merged_manifests_.erase(bin); + } + for (const auto& manifest : bin) { + if (snapshot_id_supplier_() != manifest.added_snapshot_id) { + --replaced_manifests_count_; + } + } + } + } + return {}; +} - std::vector result; - result.reserve(all.size()); - for (auto& [spec_id, group] : by_spec) { - std::ignore = spec_id; - ICEBERG_ASSIGN_OR_RAISE(auto merged, - MergeGroup(group, first, metadata, writer_factory)); - std::ranges::move(merged, std::back_inserter(result)); +ManifestMergeManager::ManifestGroups ManifestMergeManager::GroupBySpec( + const std::vector& manifests) { + ManifestGroups groups; + for (const auto& manifest : manifests) { + groups[manifest.partition_spec_id].push_back(manifest); } - return result; + return groups; } Result> ManifestMergeManager::MergeGroup( - const std::vector& group, const ManifestFile* first, + const ManifestFile& first, int32_t spec_id, const std::vector& group, const TableMetadata& metadata, const ManifestWriterFactory& writer_factory) { - // Match packEnd(group, ManifestFile::length) with lookback 1: - // 1. Process manifests in reverse order (oldest-first). - // 2. Greedy forward-pack with lookback=1: emit the current bin when the next item - // doesn't fit, then start a new bin. - // 3. Reverse each bin (restoring original item order within a bin). - // 4. Reverse the bin list (newest manifest's bin ends up first). - // Effect: the newest manifest is in the first, possibly under-filled, bin. - std::vector> bins; - std::vector current_bin; - int64_t bin_size = 0; - - for (const auto* manifest : std::views::reverse(group)) { + // Use a lookback of 1 to avoid reordering the manifests. Using 1 also means this + // should pack from the end so that the manifest that gets under-filled is the + // first one, which will be merged the next time. + // This is ListPacker(targetSizeBytes, 1, false).packEnd(group, ManifestFile::length). + std::vector> bins; + std::vector current_bin; + int64_t bin_weight = 0; + for (const auto& manifest : std::views::reverse(group)) { if (!current_bin.empty() && - bin_size + manifest->manifest_length > target_size_bytes_) { + bin_weight + manifest.manifest_length > target_size_bytes_) { + std::ranges::reverse(current_bin); bins.push_back(std::move(current_bin)); current_bin.clear(); - bin_size = 0; + bin_weight = 0; } current_bin.push_back(manifest); - bin_size += manifest->manifest_length; + bin_weight += manifest.manifest_length; } if (!current_bin.empty()) { + std::ranges::reverse(current_bin); bins.push_back(std::move(current_bin)); } - - for (auto& bin : bins) { - std::ranges::reverse(bin); - } std::ranges::reverse(bins); - // Process each bin: if the bin contains the newest manifest and is too small, - // pass its contents through unchanged. - std::vector result; - result.reserve(group.size()); - for (auto& bin : bins) { - if (bin.size() == 1) { - result.push_back(*bin[0]); - } else if (bool contains_first = std::ranges::find(bin, first) != bin.end(); - contains_first && std::cmp_less(bin.size(), min_count_to_merge_)) { - for (const auto* manifest : bin) { - result.push_back(*manifest); + // Process bins in parallel, but put results in the order of the bins into an + // array to preserve the order of manifests and contents. Preserving the order + // helps avoid random deletes when data files are eventually aged off. + std::vector> bin_results(bins.size()); + std::atomic failed{false}; + auto tasks = TaskGroup().SetExecutor(worker_pool_supplier_ ? worker_pool_supplier_() + : OptionalExecutor{}); + for (size_t index = 0; index < bins.size(); ++index) { + tasks.Submit([&, index]() -> Status { + if (failed.load()) { + return {}; } - } else { - const auto* cached = merged_manifests_.Find(bin); - if (cached != nullptr) { - result.push_back(*cached); + const auto& bin = bins[index]; + auto& output_manifests = bin_results[index]; + if (bin.size() == 1) { + // No need to rewrite. + output_manifests.push_back(bin.front()); + return {}; + } + + // If the bin has the first manifest (the new data files or an appended + // manifest file), then only merge it if the number of manifests is above + // the minimum count. This is applied only to bins with an in-memory + // manifest so that large manifests don't prevent merging older groups. + if (std::ranges::find(bin, first) != bin.end() && + std::cmp_less(bin.size(), min_count_to_merge_)) { + // Not enough to merge, add all manifest files to the output list. + output_manifests = bin; } else { - const int64_t snapshot_id = snapshot_id_supplier_(); - ICEBERG_ASSIGN_OR_RAISE(auto merged, FlushBin(bin, metadata, writer_factory)); - merged_manifests_.Add(bin, merged); - for (const auto* manifest : bin) { - if (!manifest->added_snapshot_id.has_value() || - manifest->added_snapshot_id.value() != snapshot_id) { - ++replaced_manifests_count_; - } + // Merge the bin into a single manifest. + auto manifest = CreateManifest(spec_id, bin, metadata, writer_factory); + if (!manifest) { + failed.store(true); + return ::iceberg::unexpected(std::move(manifest.error())); } - result.push_back(std::move(merged)); + output_manifests.push_back(std::move(manifest.value())); } - } + return {}; + }); } - - return result; + ICEBERG_RETURN_UNEXPECTED(std::move(tasks).Run()); + return bin_results | std::views::join | std::views::as_rvalue | + std::ranges::to>(); } -Result ManifestMergeManager::FlushBin( - const std::vector& bin, const TableMetadata& metadata, +Result ManifestMergeManager::CreateManifest( + int32_t spec_id, const std::vector& bin, const TableMetadata& metadata, const ManifestWriterFactory& writer_factory) { - const ManifestFile& first = *bin[0]; - int32_t spec_id = first.partition_spec_id; + // If this merge was already rewritten, use the existing file. If the new files + // are in this merge, then the ManifestFile for the new files has changed and + // will be a cache miss. + { + std::lock_guard lock(merged_manifests_mutex_); + auto cached = merged_manifests_.find(bin); + if (cached != merged_manifests_.end()) { + return cached->second; + } + } - ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); ICEBERG_ASSIGN_OR_RAISE(auto spec, metadata.PartitionSpecById(spec_id)); - + ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); ICEBERG_ASSIGN_OR_RAISE(auto writer, writer_factory(spec_id, manifest_content_)); - - const int64_t snapshot_id = snapshot_id_supplier_(); - for (const auto* manifest : bin) { - bool is_committed = manifest->added_snapshot_id.has_value() && - manifest->added_snapshot_id.value() != snapshot_id; - ICEBERG_ASSIGN_OR_RAISE(auto reader, ManifestReader::Make(*manifest, file_io_, schema, - spec, is_committed)); - ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->Entries()); - for (const auto& entry : entries) { - bool is_current = - entry.snapshot_id.has_value() && entry.snapshot_id.value() == snapshot_id; - if (entry.status == ManifestStatus::kDeleted) { - // Carry forward only the current snapshot's deletes; drop older tombstones. - if (is_current) { - ICEBERG_RETURN_UNEXPECTED(writer->WriteDeletedEntry(entry)); + auto write_entries = [&]() -> Status { + for (const auto& manifest : bin) { + bool is_committed = manifest.added_snapshot_id.has_value() && + snapshot_id_supplier_() != manifest.added_snapshot_id; + ICEBERG_ASSIGN_OR_RAISE( + auto reader, + ManifestReader::Make(manifest, file_io_, schema, spec, is_committed)); + ICEBERG_ASSIGN_OR_RAISE(auto entries, reader->Entries()); + for (const auto& entry : entries) { + if (entry.status == ManifestStatus::kDeleted) { + // Suppress deletes from previous snapshots. Only files deleted by this + // snapshot should be added to the new manifest. + if (entry.snapshot_id == snapshot_id_supplier_()) { + ICEBERG_RETURN_UNEXPECTED(writer->WriteDeletedEntry(entry)); + } + } else if (entry.status == ManifestStatus::kAdded && + entry.snapshot_id == snapshot_id_supplier_()) { + // Adds from this snapshot are still adds, otherwise they should be existing. + ICEBERG_RETURN_UNEXPECTED(writer->WriteAddedEntry(entry)); + } else { + // Add all files from the old manifest as existing files. + ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); } - } else if (entry.status == ManifestStatus::kAdded && is_current) { - // Files added by the current snapshot retain their ADDED status. - ICEBERG_RETURN_UNEXPECTED(writer->WriteAddedEntry(entry)); - } else { - // Files added by prior snapshots (ADDED or EXISTING) become EXISTING. - ICEBERG_RETURN_UNEXPECTED(writer->WriteExistingEntry(entry)); } } + return {}; + }; + auto status = write_entries(); + // Java Exceptions.close(writer, threw): always close, preserving the original + // read/write failure if closing fails as well. + auto close_status = writer->Close(); + ICEBERG_RETURN_UNEXPECTED(status); + ICEBERG_RETURN_UNEXPECTED(close_status); + ICEBERG_ASSIGN_OR_RAISE(auto manifest, writer->ToManifestFile()); + + // Cache the merged manifest to reuse when retrying and track replaced manifests. + { + std::lock_guard lock(merged_manifests_mutex_); + merged_manifests_.insert_or_assign(bin, manifest); } - - ICEBERG_RETURN_UNEXPECTED(writer->Close()); - return writer->ToManifestFile(); -} - -ManifestMergeManager::MergedManifestCache::Key -ManifestMergeManager::MergedManifestCache::MakeKey( - const std::vector& bin) { - Key key; - key.bin.reserve(bin.size()); - for (const auto* manifest : bin) { - key.bin.push_back(*manifest); - } - return key; -} - -const ManifestFile* ManifestMergeManager::MergedManifestCache::Find( - const std::vector& bin) const { - auto iter = entries_.find(MakeKey(bin)); - if (iter == entries_.end()) { - return nullptr; + for (const auto& input : bin) { + // Only count manifests from previous snapshots; in-memory manifests are not replaced. + if (snapshot_id_supplier_() != input.added_snapshot_id) { + ++replaced_manifests_count_; + } } - return &iter->second; + return manifest; } -void ManifestMergeManager::MergedManifestCache::Add( - const std::vector& bin, const ManifestFile& manifest) { - entries_.emplace(MakeKey(bin), manifest); -} - -size_t ManifestMergeManager::MergedManifestCache::KeyHash::operator()( - const Key& key) const { +size_t ManifestMergeManager::BinHash::operator()( + const std::vector& bin) const { size_t hash = 0; - for (const auto& manifest : key.bin) { - hash = CombineHash(hash, std::hash{}(manifest.manifest_path)); + for (const auto& manifest : bin) { + hash ^= std::hash{}(manifest) + 0x9e3779b9 + (hash << 6) + (hash >> 2); } return hash; } -Result ManifestMergeManager::MergedManifestCache::CleanUncommitted( - const std::unordered_set& committed, int64_t snapshot_id, - const std::function& delete_file) { - int32_t removed_replaced_manifests_count = 0; - auto cached_entries = - std::vector>{entries_.begin(), entries_.end()}; - for (const auto& [bin, merged] : cached_entries) { - if (committed.contains(merged.manifest_path)) { - continue; - } - - std::ignore = delete_file(merged.manifest_path); - for (const auto& manifest : bin.bin) { - if (!manifest.added_snapshot_id.has_value() || - manifest.added_snapshot_id.value() != snapshot_id) { - ++removed_replaced_manifests_count; - } - } - entries_.erase(bin); - } - return removed_replaced_manifests_count; -} - -Status ManifestMergeManager::CleanUncommitted( - const std::unordered_set& committed) { - if (merged_manifests_.empty()) { - return {}; - } - const int64_t snapshot_id = snapshot_id_supplier_(); - ICEBERG_ASSIGN_OR_RAISE( - auto removed_replaced_manifests_count, - merged_manifests_.CleanUncommitted(committed, snapshot_id, delete_file_)); - replaced_manifests_count_ -= removed_replaced_manifests_count; - return {}; -} - } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_merge_manager.h b/src/iceberg/manifest/manifest_merge_manager.h index 031c314202..98a3135cab 100644 --- a/src/iceberg/manifest/manifest_merge_manager.h +++ b/src/iceberg/manifest/manifest_merge_manager.h @@ -22,9 +22,12 @@ /// \file iceberg/manifest/manifest_merge_manager.h /// Merges small manifests into fewer larger ones according to table properties. +#include #include #include +#include #include +#include #include #include #include @@ -35,6 +38,7 @@ #include "iceberg/manifest/manifest_writer.h" #include "iceberg/result.h" #include "iceberg/type_fwd.h" +#include "iceberg/util/executor.h" namespace iceberg { @@ -50,25 +54,38 @@ namespace iceberg { class ICEBERG_EXPORT ManifestMergeManager { public: using SnapshotIdSupplier = std::function; + using WorkerPoolSupplier = std::function; /// \brief Construct a merge manager with the given configuration. /// /// \param content Manifest content this manager accepts /// \param target_size_bytes Target output manifest size in bytes - /// \param min_count_to_merge Minimum number of manifests before any merging occurs + /// \param min_count_to_merge Minimum count for the bin containing the first manifest /// \param merge_enabled Whether merging is enabled at all /// \param file_io File IO used to open manifests for reading /// \param snapshot_id_supplier Supplies the snapshot id being committed /// \param delete_file Callback that deletes uncommitted merged manifest files + /// \param worker_pool_supplier Supplies the executor for parallel bin merging; + /// an empty supplier runs on the calling thread. Callbacks must support + /// concurrent calls when an executor is supplied. static Result> Make( ManifestContent content, int64_t target_size_bytes, int32_t min_count_to_merge, bool merge_enabled, std::shared_ptr file_io, SnapshotIdSupplier snapshot_id_supplier, - std::function delete_file = {}); + std::function delete_file = {}, + WorkerPoolSupplier worker_pool_supplier = {}); ManifestMergeManager(const ManifestMergeManager&) = delete; ManifestMergeManager& operator=(const ManifestMergeManager&) = delete; + /// \brief Merge manifests in their supplied order, with new manifests first. + /// + /// The first manifest's bin is protected by min_count_to_merge. Groups are + /// processed in descending spec ID order and bins preserve manifest order. + Result> MergeManifests( + const std::vector& manifests, const TableMetadata& metadata, + const ManifestWriterFactory& writer_factory); + /// \brief Merge existing and new manifests according to configured thresholds. /// /// Manifests are grouped by partition_spec_id. Within each group, a greedy @@ -87,7 +104,7 @@ class ICEBERG_EXPORT ManifestMergeManager { const ManifestWriterFactory& writer_factory); /// \brief Returns the number of manifests replaced by cached merged outputs. - int32_t ReplacedManifestsCount() const { return replaced_manifests_count_; } + int32_t ReplacedManifestsCount() const { return replaced_manifests_count_.load(); } /// \brief Delete cached merged manifests whose paths were not committed and roll /// back replaced-manifest accounting. @@ -98,49 +115,25 @@ class ICEBERG_EXPORT ManifestMergeManager { int32_t min_count_to_merge, bool merge_enabled, std::shared_ptr file_io, SnapshotIdSupplier snapshot_id_supplier, - std::function delete_file); - - struct MergedManifestCache { - struct Key { - std::vector bin; - bool operator==(const Key& other) const = default; - }; + std::function delete_file, + WorkerPoolSupplier worker_pool_supplier); - struct KeyHash { - size_t operator()(const Key& key) const; - }; + using ManifestGroups = std::map, std::greater<>>; - const ManifestFile* Find(const std::vector& bin) const; - void Add(const std::vector& bin, const ManifestFile& manifest); - bool empty() const { return entries_.empty(); } - Result CleanUncommitted( - const std::unordered_set& committed, int64_t snapshot_id, - const std::function& delete_file); - - private: - static Key MakeKey(const std::vector& bin); - - std::unordered_map entries_; + struct BinHash { + size_t operator()(const std::vector& bin) const; }; - /// \brief Merge a group of manifests sharing the same spec_id. - /// - /// \param first The overall first (newest) manifest across all groups, used to - /// apply the min_count_to_merge threshold on the bin that contains it. + static ManifestGroups GroupBySpec(const std::vector& manifests); + Result> MergeGroup( - const std::vector& group, const ManifestFile* first, + const ManifestFile& first, int32_t spec_id, const std::vector& group, const TableMetadata& metadata, const ManifestWriterFactory& writer_factory); - /// \brief Write a merged manifest from all manifests in a bin. - /// - /// Entries are written snapshot-aware: - /// - ADDED from snapshot_id → WriteAddedEntry (preserve status) - /// - DELETED from snapshot_id → WriteDeletedEntry (preserve tombstone) - /// - DELETED from older snapshots → dropped (stale tombstones are not carried forward) - /// - All other entries → WriteExistingEntry - Result FlushBin(const std::vector& bin, - const TableMetadata& metadata, - const ManifestWriterFactory& writer_factory); + Result CreateManifest(int32_t spec_id, + const std::vector& bin, + const TableMetadata& metadata, + const ManifestWriterFactory& writer_factory); const ManifestContent manifest_content_; const int64_t target_size_bytes_; @@ -149,8 +142,12 @@ class ICEBERG_EXPORT ManifestMergeManager { std::shared_ptr file_io_; SnapshotIdSupplier snapshot_id_supplier_; std::function delete_file_; - int32_t replaced_manifests_count_{0}; - MergedManifestCache merged_manifests_; + // Track manifests replaced during bin-packing. + std::atomic replaced_manifests_count_{0}; + // Cache merge results to reuse when retrying. + std::unordered_map, ManifestFile, BinHash> merged_manifests_; + std::mutex merged_manifests_mutex_; + WorkerPoolSupplier worker_pool_supplier_; }; } // namespace iceberg diff --git a/src/iceberg/test/manifest_filter_manager_test.cc b/src/iceberg/test/manifest_filter_manager_test.cc index 442bbe0635..8fd98dc8ef 100644 --- a/src/iceberg/test/manifest_filter_manager_test.cc +++ b/src/iceberg/test/manifest_filter_manager_test.cc @@ -19,6 +19,7 @@ #include "iceberg/manifest/manifest_filter_manager.h" +#include #include #include #include @@ -38,6 +39,7 @@ #include "iceberg/schema.h" #include "iceberg/table.h" #include "iceberg/table_metadata.h" +#include "iceberg/test/executor.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" #include "iceberg/update/fast_append.h" @@ -119,6 +121,12 @@ class ManifestFilterManagerTest : public MinimalUpdateTestBase { return result; } + Result> Manifests(const Snapshot& snapshot) { + ICEBERG_ASSIGN_OR_RAISE(auto reader, + ManifestListReader::Make(snapshot.manifest_list, file_io_)); + return reader->Files(); + } + static constexpr int64_t kTestSnapshotId = 55555L; int manifest_counter_ = 0; std::shared_ptr spec_; @@ -742,8 +750,8 @@ TEST_F(ManifestFilterManagerTest, DropDeleteFilesOlderThanDuringDeleteManifestRe } TEST_F(ManifestFilterManagerTest, FailAnyDeleteFailsOnSequenceNumberPruning) { - // An unrelated object delete opens the manifest; sequence-number pruning is the - // branch that marks this entry. + // An unrelated object delete in the same partition opens the manifest; sequence-number + // pruning is the branch that marks this entry. auto* metadata = table_->metadata().get(); auto factory = MakeWriterFactory(*metadata); @@ -762,12 +770,12 @@ TEST_F(ManifestFilterManagerTest, FailAnyDeleteFailsOnSequenceNumberPruning) { auto del_manifest, WriteDeleteManifest(old_file, /*data_seq=*/2L, file_io_, *metadata, manifest_path)); - // Unrelated registered delete file: opens the manifest without matching its entry. + // The partition overlaps the manifest summary but the file identity is unrelated. auto unrelated = std::make_shared(); unrelated->content = DataFile::Content::kPositionDeletes; unrelated->file_path = table_location_ + "/delete/unrelated.parquet"; unrelated->file_format = FileFormatType::kParquet; - unrelated->partition = PartitionValues(std::vector{Literal::Long(9L)}); + unrelated->partition = PartitionValues(std::vector{Literal::Long(1L)}); unrelated->file_size_in_bytes = 512; unrelated->record_count = 10; unrelated->partition_spec_id = spec_->spec_id(); @@ -1190,4 +1198,380 @@ TEST_F(ManifestFilterManagerTest, DuplicateDeletesCountDoesNotCrossManifestBound EXPECT_EQ(summary_map.count(SnapshotSummaryFields::kDeletedDuplicatedFiles), 0U); } +// Java TestDeleteFiles and ManifestFilterManager.canTrustManifestReferences: +// a file read from a manifest carries its origin, enabling exact manifest pruning. +TEST_F(ManifestFilterManagerTest, TrustedManifestReferencesSkipUnrelatedManifests) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(manifests, *metadata)); + ASSERT_EQ(entries.size(), 1U); + EXPECT_EQ(entries[0].data_file->manifest_location, manifests[0].manifest_path); + auto unrelated = manifests[0]; + unrelated.manifest_path = table_location_ + "/missing-unrelated.avro"; + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(entries[0].data_file), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&unrelated, &manifests[0]}, + MakeWriterFactory(*metadata))); + ASSERT_EQ(filtered.size(), 2U); + EXPECT_EQ(filtered[0], unrelated); + EXPECT_NE(filtered[1], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, MissingManifestReferenceFallsBackToPartitionPruning) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + auto file = std::make_shared(*file_a_); + file->manifest_location = "old-rewritten-manifest.avro"; + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(file), IsOk()); + manager->FailMissingDeletePaths(); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&manifests[0]}, + MakeWriterFactory(*metadata))); + EXPECT_NE(filtered[0], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, PathDeleteDisablesTrustedManifestReferences) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + ICEBERG_UNWRAP_OR_FAIL(auto first, Manifests(*snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto next_snapshot, CommitFiles({file_b_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto all, Manifests(*next_snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(first, *metadata)); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(entries[0].data_file), IsOk()); + ASSERT_THAT(manager->DeleteFile(file_b_->file_path), IsOk()); + manager->FailMissingDeletePaths(); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, next_snapshot, MakeWriterFactory(*metadata))); + EXPECT_EQ(manager->DeletedFiles().size(), 2U); + EXPECT_EQ(manager->ReplacedManifestsCount(), 2); +} + +TEST_F(ManifestFilterManagerTest, ObjectDeletePrunesNonMatchingPartitionBounds) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ASSERT_THAT(file_io_->DeleteFile(manifests[0].manifest_path), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(file_b_), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&manifests[0]}, + MakeWriterFactory(*metadata))); + EXPECT_EQ(filtered[0], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, DropPartitionPrunesNonMatchingPartitionBounds) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ASSERT_THAT(file_io_->DeleteFile(manifests[0].manifest_path), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DropPartition(spec_->spec_id(), file_b_->partition), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&manifests[0]}, + MakeWriterFactory(*metadata))); + EXPECT_EQ(filtered[0], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, EmptyFileDoesNotMatchRowFilter) { + file_a_->record_count = 0; + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteByRowFilter(Expressions::AlwaysTrue()), IsOk()); + manager->FailAnyDelete(); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + EXPECT_EQ(filtered, manifests); + EXPECT_TRUE(manager->DeletedFiles().empty()); +} + +TEST_F(ManifestFilterManagerTest, DeletedFilesAreCumulativeAndRequiredOnRetry) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteByRowFilter(Expressions::AlwaysTrue()), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + ASSERT_EQ(manager->FilesToBeDeleted().size(), 1U); + manager->FailMissingDeletePaths(); + EXPECT_THAT(manager->FilterManifests(*metadata, nullptr, MakeWriterFactory(*metadata)), + IsError(ErrorKind::kValidationFailed)); + EXPECT_EQ(manager->FilesToBeDeleted().size(), 1U); +} + +TEST_F(ManifestFilterManagerTest, DeletedCopiesDropColumnStatsAndPreserveMetadata) { + file_a_->column_sizes = {{1, 12}}; + file_a_->value_counts = {{1, 100}}; + file_a_->null_value_counts = {{1, 0}}; + file_a_->nan_value_counts = {{1, 0}}; + ICEBERG_UNWRAP_OR_FAIL(file_a_->lower_bounds[1], Literal::Long(1).Serialize()); + ICEBERG_UNWRAP_OR_FAIL(file_a_->upper_bounds[1], Literal::Long(1).Serialize()); + file_a_->split_offsets = {4}; + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(file_a_->file_path), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + ASSERT_EQ(manager->DeletedFiles().size(), 1U); + const auto& file = *manager->DeletedFiles()[0]; + EXPECT_TRUE(file.column_sizes.empty()); + EXPECT_TRUE(file.value_counts.empty()); + EXPECT_TRUE(file.null_value_counts.empty()); + EXPECT_TRUE(file.nan_value_counts.empty()); + EXPECT_TRUE(file.lower_bounds.empty()); + EXPECT_TRUE(file.upper_bounds.empty()); + EXPECT_EQ(file.record_count, file_a_->record_count); + EXPECT_EQ(file.split_offsets, file_a_->split_offsets); + EXPECT_EQ(file.manifest_location, manifests[0].manifest_path); + EXPECT_FALSE(file_a_->column_sizes.empty()); +} + +TEST_F(ManifestFilterManagerTest, SummaryOnlyCountsDuplicatesInSuppliedManifests) { + auto metadata = table_->metadata(); + auto file = std::make_shared(*file_a_); + file->content = DataFile::Content::kPositionDeletes; + ICEBERG_UNWRAP_OR_FAIL( + auto duplicate, WriteDeleteManifest({{file, 3L}, {file, 3L}}, file_io_, *metadata, + table_location_ + "/duplicate.avro")); + ICEBERG_UNWRAP_OR_FAIL(auto unique, + WriteDeleteManifest(file, 3L, file_io_, *metadata, + table_location_ + "/unique.avro")); + ICEBERG_UNWRAP_OR_FAIL( + auto manager, ManifestFilterManager::Make(ManifestContent::kDeletes, file_io_)); + ASSERT_THAT(manager->DeleteFile(file), IsOk()); + auto specs = SpecsById(*metadata); + ICEBERG_UNWRAP_OR_FAIL(auto filtered, + manager->FilterManifests(schema_, specs, {&duplicate, &unique}, + MakeWriterFactory(*metadata))); + ASSERT_EQ(filtered.size(), 2U); + ICEBERG_UNWRAP_OR_FAIL(auto summary, manager->BuildSummary({filtered[1]}, specs)); + EXPECT_EQ(summary.Build().count(SnapshotSummaryFields::kDeletedDuplicatedFiles), 0U); + ICEBERG_UNWRAP_OR_FAIL(auto empty_summary, manager->BuildSummary({}, specs)); + EXPECT_EQ(empty_summary.Build().count(SnapshotSummaryFields::kDeletedDuplicatedFiles), + 0U); + ICEBERG_UNWRAP_OR_FAIL(auto duplicate_summary, + manager->BuildSummary({filtered[0]}, specs)); + EXPECT_EQ(duplicate_summary.Build().at(SnapshotSummaryFields::kDeletedDuplicatedFiles), + "1"); +} + +TEST_F(ManifestFilterManagerTest, ReferencedManifestSkipsDetectionPass) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(manifests, *metadata)); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(entries[0].data_file), IsOk()); + // Java checks failAnyDelete only in the detection pass; known references skip it. + manager->FailAnyDelete(); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + EXPECT_NE(filtered[0], manifests[0]); + EXPECT_EQ(manager->DeletedFiles().size(), 1U); +} + +TEST_F(ManifestFilterManagerTest, ParallelFilteringPreservesManifestOrderAndRetryCache) { + ICEBERG_UNWRAP_OR_FAIL(auto first, CommitFiles({file_a_})); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_b_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + test::ThreadExecutor executor; + ICEBERG_UNWRAP_OR_FAIL(auto manager, ManifestFilterManager::Make( + ManifestContent::kData, file_io_, {}, + [&] { return OptionalExecutor{executor}; })); + ASSERT_THAT(manager->DeleteByRowFilter(Expressions::AlwaysTrue()), IsOk()); + std::atomic counter{0}; + auto factory = [&](int32_t spec_id, + ManifestContent content) -> Result> { + ICEBERG_ASSIGN_OR_RAISE(auto spec, metadata->PartitionSpecById(spec_id)); + return ManifestWriter::MakeWriter( + metadata->format_version, kTestSnapshotId, + std::format("{}/parallel-{}.avro", table_location_, counter.fetch_add(1)), + file_io_, spec, schema_, content); + }; + ICEBERG_UNWRAP_OR_FAIL(auto filtered, + manager->FilterManifests(*metadata, snapshot, factory)); + ASSERT_EQ(filtered.size(), manifests.size()); + for (size_t i = 0; i < manifests.size(); ++i) { + ICEBERG_UNWRAP_OR_FAIL(auto original_entries, + ReadAllEntries({manifests[i]}, *metadata)); + ICEBERG_UNWRAP_OR_FAIL(auto filtered_entries, + ReadAllEntries({filtered[i]}, *metadata)); + ASSERT_EQ(filtered_entries.size(), original_entries.size()); + EXPECT_EQ(filtered_entries[0].data_file->file_path, + original_entries[0].data_file->file_path); + } + ICEBERG_UNWRAP_OR_FAIL(auto retry, + manager->FilterManifests(*metadata, snapshot, factory)); + EXPECT_EQ(retry, filtered); + EXPECT_EQ(counter.load(), 2); + EXPECT_EQ(manager->ReplacedManifestsCount(), 2); + EXPECT_EQ(manager->DeletedFiles().size(), 2U); + EXPECT_EQ(executor.submit_count(), 4); +} + +TEST_F(ManifestFilterManagerTest, FailedPassDoesNotAddPartialDeletesToCumulativeSet) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + auto missing = manifests[0]; + missing.manifest_path = table_location_ + "/missing.avro"; + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteByRowFilter(Expressions::AlwaysTrue()), IsOk()); + EXPECT_FALSE(manager + ->FilterManifests(schema_, SpecsById(*metadata), + {&manifests[0], &missing}, + MakeWriterFactory(*metadata)) + .has_value()); + EXPECT_TRUE(manager->FilesToBeDeleted().empty()); + EXPECT_EQ(manager->ReplacedManifestsCount(), 1); + ICEBERG_UNWRAP_OR_FAIL( + auto retry, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + EXPECT_EQ(manager->FilesToBeDeleted().size(), 1U); + EXPECT_EQ(manager->ReplacedManifestsCount(), 1); +} + +TEST_F(ManifestFilterManagerTest, FailureStopsPendingSerialManifestWork) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + auto missing = manifests[0]; + missing.manifest_path = table_location_ + "/missing.avro"; + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteByRowFilter(Expressions::AlwaysTrue()), IsOk()); + EXPECT_FALSE(manager + ->FilterManifests(schema_, SpecsById(*metadata), + {&missing, &manifests[0]}, + MakeWriterFactory(*metadata)) + .has_value()); + EXPECT_EQ(manager->ReplacedManifestsCount(), 0); + EXPECT_EQ(manifest_counter_, 0); +} + +TEST_F(ManifestFilterManagerTest, ManifestProvenanceDoesNotChangeDataFileEquality) { + DataFile left = *file_a_; + DataFile right = left; + left.manifest_location = "original.avro"; + right.manifest_location = "rewritten.avro"; + EXPECT_EQ(left, right); + auto copy = left.CopyWithoutStats(); + EXPECT_EQ(copy->manifest_location, left.manifest_location); + right.record_count += 1; + EXPECT_NE(left, right); +} + +TEST_F(ManifestFilterManagerTest, DropNullPartitionMatchesNullTuple) { + file_a_->partition = PartitionValues(Literal::Null(int64())); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_, file_b_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DropPartition(spec_->spec_id(), file_a_->partition), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(*metadata, snapshot, MakeWriterFactory(*metadata))); + ASSERT_EQ(manager->DeletedFiles().size(), 1U); + EXPECT_EQ(manager->DeletedFiles()[0]->file_path, file_a_->file_path); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(filtered, *metadata)); + EXPECT_EQ(std::ranges::count_if(entries, + [](const auto& entry) { + return entry.status == ManifestStatus::kExisting; + }), + 1); +} + +TEST_F(ManifestFilterManagerTest, NullPartitionSummaryPrunesNonNullObjectDelete) { + file_a_->partition = PartitionValues(Literal::Null(int64())); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ASSERT_THAT(file_io_->DeleteFile(manifests[0].manifest_path), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DeleteFile(file_b_), IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&manifests[0]}, + MakeWriterFactory(*metadata))); + EXPECT_EQ(filtered[0], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, NonNullPartitionSummaryPrunesNullDrop) { + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, CommitFiles({file_a_})); + auto metadata = table_->metadata(); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, Manifests(*snapshot)); + ASSERT_THAT(file_io_->DeleteFile(manifests[0].manifest_path), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT( + manager->DropPartition(spec_->spec_id(), PartitionValues(Literal::Null(int64()))), + IsOk()); + ICEBERG_UNWRAP_OR_FAIL( + auto filtered, + manager->FilterManifests(schema_, SpecsById(*metadata), {&manifests[0]}, + MakeWriterFactory(*metadata))); + EXPECT_EQ(filtered[0], manifests[0]); +} + +TEST_F(ManifestFilterManagerTest, DropNaNPartitionMatchesDifferentNaNPayload) { + auto schema = std::make_shared(std::vector{ + SchemaField::MakeOptional(spec_->fields()[0].source_id(), "x", float64())}); + const double positive_nan = std::bit_cast(uint64_t{0x7ff8000000000001}); + const double negative_nan = std::bit_cast(uint64_t{0xfff8000000000002}); + auto file = std::make_shared(*file_a_); + file->partition = PartitionValues(Literal::Double(positive_nan)); + ICEBERG_UNWRAP_OR_FAIL(auto writer, ManifestWriter::MakeWriter( + 2, 123, table_location_ + "/metadata/nan.avro", + file_io_, spec_, schema)); + ASSERT_THAT(writer->WriteAddedEntry(file), IsOk()); + ASSERT_THAT(writer->Close(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifest, writer->ToManifestFile()); + manifest.sequence_number = 1; + ICEBERG_UNWRAP_OR_FAIL(auto manager, + ManifestFilterManager::Make(ManifestContent::kData, file_io_)); + ASSERT_THAT(manager->DropPartition(spec_->spec_id(), + PartitionValues(Literal::Double(negative_nan))), + IsOk()); + auto factory = [&](int32_t, ManifestContent content) { + return ManifestWriter::MakeWriter(2, kTestSnapshotId, + table_location_ + "/metadata/nan-filtered.avro", + file_io_, spec_, schema, content); + }; + ICEBERG_UNWRAP_OR_FAIL(auto filtered, + manager->FilterManifests(schema, {{spec_->spec_id(), spec_}}, + {&manifest}, factory)); + ASSERT_EQ(filtered.size(), 1U); + ASSERT_EQ(manager->DeletedFiles().size(), 1U); + EXPECT_EQ(manager->DeletedFiles()[0]->file_path, file->file_path); +} + } // namespace iceberg diff --git a/src/iceberg/test/manifest_merge_manager_test.cc b/src/iceberg/test/manifest_merge_manager_test.cc index 18d24db29f..e19272c41f 100644 --- a/src/iceberg/test/manifest_merge_manager_test.cc +++ b/src/iceberg/test/manifest_merge_manager_test.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -42,6 +43,7 @@ #include "iceberg/schema_field.h" #include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" +#include "iceberg/test/executor.h" #include "iceberg/test/matchers.h" #include "iceberg/transform.h" #include "iceberg/type.h" @@ -526,4 +528,250 @@ TEST_F(ManifestMergeManagerTest, PackEndOlderManifestsMergedNotNewest) { EXPECT_EQ(count, 3); } +TEST_F(ManifestMergeManagerTest, EmptyAndDisabledMergeDoNotUseWorkerPool) { + test::InlineExecutor executor; + int supplier_calls = 0; + auto supplier = [&]() -> OptionalExecutor { + ++supplier_calls; + return std::ref(executor); + }; + ICEBERG_UNWRAP_OR_FAIL(auto enabled, + ManifestMergeManager::Make( + ManifestContent::kData, 1000, 2, true, file_io_, + [] { return kSnapshotId; }, {}, supplier)); + ICEBERG_UNWRAP_OR_FAIL(auto empty, + enabled->MergeManifests({}, *metadata_, MakeWriterFactory())); + EXPECT_TRUE(empty.empty()); + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1)); + ICEBERG_UNWRAP_OR_FAIL(auto m1, WriteManifest(kSpecId1, 1)); + ICEBERG_UNWRAP_OR_FAIL(auto disabled, + ManifestMergeManager::Make( + ManifestContent::kData, 1000, 2, false, file_io_, + [] { return kSnapshotId; }, {}, supplier)); + const std::vector manifests{m0, m1}; + ICEBERG_UNWRAP_OR_FAIL(auto unchanged, disabled->MergeManifests(manifests, *metadata_, + MakeWriterFactory())); + EXPECT_EQ(unchanged, manifests); + EXPECT_EQ(supplier_calls, 0); + EXPECT_EQ(executor.submit_count(), 0); + EXPECT_EQ(factory_call_count_, 0); +} + +// Java TestMergeAppend.testManifestMergeMinCount and ManifestMergeManager.mergeGroup: +// threshold protection follows ManifestFile equality, including repeated descriptors. +TEST_F(ManifestMergeManagerTest, MinCountUsesManifestEqualityAcrossBins) { + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m1, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m2, WriteManifest(kSpecId0, 1, 100)); + const std::vector manifests{m0, m1, m0, m2}; + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(200, 3, true)); + ICEBERG_UNWRAP_OR_FAIL(auto result, + mgr->MergeManifests(manifests, *metadata_, MakeWriterFactory())); + EXPECT_EQ(result, manifests); + EXPECT_EQ(factory_call_count_, 0); +} + +TEST_F(ManifestMergeManagerTest, GroupsNewestSpecFirstWithoutReorderingFiles) { + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m1, WriteManifest(kSpecId1, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m2, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m3, WriteManifest(kSpecId1, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(1000, 3, true)); + ICEBERG_UNWRAP_OR_FAIL(auto result, mgr->MergeManifests({m0, m1, m2, m3}, *metadata_, + MakeWriterFactory())); + ASSERT_EQ(result.size(), 3U); + EXPECT_EQ(result[0].partition_spec_id, kSpecId1); + EXPECT_EQ(result[1], m0); + EXPECT_EQ(result[2], m2); + ICEBERG_UNWRAP_OR_FAIL(auto reader, + ManifestReader::Make(result[0], file_io_, schema_, spec1_)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, reader->Entries()); + ICEBERG_UNWRAP_OR_FAIL(auto first_reader, + ManifestReader::Make(m1, file_io_, schema_, spec1_)); + ICEBERG_UNWRAP_OR_FAIL(auto first_entries, first_reader->Entries()); + ICEBERG_UNWRAP_OR_FAIL(auto second_reader, + ManifestReader::Make(m3, file_io_, schema_, spec1_)); + ICEBERG_UNWRAP_OR_FAIL(auto second_entries, second_reader->Entries()); + ASSERT_EQ(entries.size(), 2U); + EXPECT_EQ(entries[0].data_file->file_path, first_entries[0].data_file->file_path); + EXPECT_EQ(entries[1].data_file->file_path, second_entries[0].data_file->file_path); +} + +TEST_F(ManifestMergeManagerTest, CacheKeyPreservesBinOrderAndManifestIdentity) { + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m1, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(1000, 2, true)); + ICEBERG_UNWRAP_OR_FAIL(auto first, + mgr->MergeManifests({m0, m1}, *metadata_, MakeWriterFactory())); + ICEBERG_UNWRAP_OR_FAIL(auto reversed, + mgr->MergeManifests({m1, m0}, *metadata_, MakeWriterFactory())); + EXPECT_NE(first[0], reversed[0]); + m0.manifest_length = 101; + ICEBERG_UNWRAP_OR_FAIL(auto retried, + mgr->MergeManifests({m0, m1}, *metadata_, MakeWriterFactory())); + EXPECT_EQ(first, retried); + EXPECT_EQ(factory_call_count_, 2); +} + +TEST_F(ManifestMergeManagerTest, ParallelBinsPreserveOrderAndCacheResults) { + std::vector manifests; + std::vector file_paths; + for (int i = 0; i < 6; ++i) { + ICEBERG_UNWRAP_OR_FAIL(auto manifest, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadEntries(manifest)); + file_paths.push_back(entries[0].data_file->file_path); + manifest.added_snapshot_id = kSnapshotId - 1; + manifests.push_back(std::move(manifest)); + } + test::ThreadExecutor executor; + ICEBERG_UNWRAP_OR_FAIL( + auto mgr, + ManifestMergeManager::Make( + ManifestContent::kData, 200, 2, true, file_io_, [] { return kSnapshotId; }, {}, + [&]() -> OptionalExecutor { return std::ref(executor); })); + std::mutex factory_mutex; + auto factory = MakeWriterFactory(); + ManifestWriterFactory synchronized_factory = [&](int32_t spec_id, + ManifestContent content) { + std::lock_guard lock(factory_mutex); + return factory(spec_id, content); + }; + ICEBERG_UNWRAP_OR_FAIL( + auto result, mgr->MergeManifests(manifests, *metadata_, synchronized_factory)); + ASSERT_EQ(result.size(), 3U); + EXPECT_EQ(executor.submit_count(), 3); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 6); + for (size_t i = 0; i < result.size(); ++i) { + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadEntries(result[i])); + ASSERT_EQ(entries.size(), 2U); + EXPECT_EQ(entries[0].data_file->file_path, file_paths[2 * i]); + EXPECT_EQ(entries[1].data_file->file_path, file_paths[2 * i + 1]); + } + ICEBERG_UNWRAP_OR_FAIL( + auto retried, mgr->MergeManifests(manifests, *metadata_, synchronized_factory)); + EXPECT_EQ(result, retried); + EXPECT_EQ(factory_call_count_, 3); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 6); + ASSERT_THAT(mgr->CleanUncommitted({result[0].manifest_path}), IsOk()); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 2); +} + +TEST_F(ManifestMergeManagerTest, StopsRemainingBinsAfterWriterFailure) { + std::vector manifests; + for (int i = 0; i < 6; ++i) { + ICEBERG_UNWRAP_OR_FAIL(auto manifest, WriteManifest(kSpecId0, 1, 100)); + manifests.push_back(std::move(manifest)); + } + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(200, 2, true)); + int calls = 0; + ManifestWriterFactory factory = + [&](int32_t, ManifestContent) -> Result> { + ++calls; + return IOError("writer failed"); + }; + EXPECT_THAT(mgr->MergeManifests(manifests, *metadata_, factory), + IsError(ErrorKind::kIOError)); + EXPECT_EQ(calls, 1); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 0); +} + +TEST_F(ManifestMergeManagerTest, ExecutorSubmissionFailureDoesNotCreateManifest) { + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto m1, WriteManifest(kSpecId0, 1, 100)); + test::ThreadExecutor executor(ServiceUnavailable("executor rejected merge")); + ICEBERG_UNWRAP_OR_FAIL( + auto mgr, + ManifestMergeManager::Make( + ManifestContent::kData, 200, 2, true, file_io_, [] { return kSnapshotId; }, {}, + [&]() -> OptionalExecutor { return std::ref(executor); })); + EXPECT_THAT(mgr->MergeManifests({m0, m1}, *metadata_, MakeWriterFactory()), + HasErrorMessage("executor rejected merge")); + EXPECT_EQ(factory_call_count_, 0); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 0); +} + +// Java createManifest closes the writer in finally even when a later reader fails. +TEST_F(ManifestMergeManagerTest, ClosesAndFlushesWriterOnReadFailureWithoutCaching) { + ICEBERG_UNWRAP_OR_FAIL(auto m0, WriteManifest(kSpecId0, 1, 100)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadEntries(m0)); + auto missing = m0; + missing.manifest_path = "missing.avro"; + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(200, 2, true)); + std::string output_path = "partial-merge.avro"; + ManifestWriterFactory factory = [&](int32_t, ManifestContent content) { + return ManifestWriter::MakeWriter(kFormatVersion, kSnapshotId, output_path, file_io_, + spec0_, schema_, content); + }; + EXPECT_FALSE(mgr->MergeManifests({m0, missing}, *metadata_, factory).has_value()); + EXPECT_EQ(mgr->ReplacedManifestsCount(), 0); + auto output = m0; + output.manifest_path = output_path; + ICEBERG_UNWRAP_OR_FAIL(auto input, file_io_->NewInputFile(output_path)); + ICEBERG_UNWRAP_OR_FAIL(output.manifest_length, input->Size()); + ICEBERG_UNWRAP_OR_FAIL(auto partial_entries, ReadEntries(output)); + ASSERT_EQ(partial_entries.size(), 1U); + EXPECT_EQ(partial_entries[0].data_file->file_path, entries[0].data_file->file_path); + ASSERT_THAT(mgr->CleanUncommitted({}), IsOk()); + // The unsuccessful writer has no cached ManifestFile; its owner handles cleanup. + ICEBERG_UNWRAP_OR_FAIL(auto remaining_input, file_io_->NewInputFile(output_path)); + EXPECT_TRUE(remaining_input->Size().has_value()); +} + +// Java TestMergeAppend validates snapshot-aware status and sequence preservation. +TEST_F(ManifestMergeManagerTest, PreservesEntryStatusAndSequenceNumbers) { + for (const auto content : {ManifestContent::kData, ManifestContent::kDeletes}) { + SCOPED_TRACE(content == ManifestContent::kData ? "data" : "deletes"); + std::vector manifests; + for (const int64_t snapshot_id : {kSnapshotId, kSnapshotId - 1}) { + auto path = std::format("statuses-{}.avro", manifest_counter_++); + ICEBERG_UNWRAP_OR_FAIL( + auto writer, ManifestWriter::MakeWriter(kFormatVersion, snapshot_id, path, + file_io_, spec0_, schema_, content)); + for (const auto status : {ManifestStatus::kAdded, ManifestStatus::kExisting, + ManifestStatus::kDeleted}) { + auto file = std::make_shared(); + file->file_path = + std::format("{}-{}.parquet", snapshot_id, static_cast(status)); + file->content = content == ManifestContent::kData + ? DataFile::Content::kData + : DataFile::Content::kPositionDeletes; + file->partition_spec_id = kSpecId0; + file->partition = PartitionValues({Literal::Long(0)}); + file->record_count = 10; + file->file_size_in_bytes = 100; + ASSERT_THAT(writer->WriteEntry(ManifestEntry{.status = status, + .snapshot_id = snapshot_id, + .sequence_number = 7, + .file_sequence_number = 8, + .data_file = file}), + IsOk()); + } + ASSERT_THAT(writer->Close(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifest, writer->ToManifestFile()); + manifest.manifest_length = 100; + manifests.push_back(std::move(manifest)); + } + ICEBERG_UNWRAP_OR_FAIL(auto mgr, MakeMergeManager(200, 2, true, content)); + ICEBERG_UNWRAP_OR_FAIL( + auto result, mgr->MergeManifests(manifests, *metadata_, MakeWriterFactory())); + ASSERT_EQ(result.size(), 1U); + auto committed = result[0]; + committed.sequence_number = 9; + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadEntries(committed)); + ASSERT_EQ(entries.size(), 5U); + EXPECT_EQ(entries[0].status, ManifestStatus::kAdded); + EXPECT_EQ(entries[0].file_sequence_number, 9); + EXPECT_EQ(entries[1].status, ManifestStatus::kExisting); + EXPECT_EQ(entries[2].status, ManifestStatus::kDeleted); + EXPECT_EQ(entries[3].status, ManifestStatus::kExisting); + EXPECT_EQ(entries[4].status, ManifestStatus::kExisting); + for (size_t i = 0; i < entries.size(); ++i) { + EXPECT_EQ(entries[i].snapshot_id, i < 3 ? kSnapshotId : kSnapshotId - 1); + EXPECT_EQ(entries[i].sequence_number, 7); + if (i != 0) EXPECT_EQ(entries[i].file_sequence_number, 8); + } + EXPECT_EQ(mgr->ReplacedManifestsCount(), 1); + } +} + } // namespace iceberg diff --git a/src/iceberg/test/merging_snapshot_update_test.cc b/src/iceberg/test/merging_snapshot_update_test.cc index d31a1eb291..16a8f58302 100644 --- a/src/iceberg/test/merging_snapshot_update_test.cc +++ b/src/iceberg/test/merging_snapshot_update_test.cc @@ -149,6 +149,8 @@ class TestMergeAppend : public MergingSnapshotUpdate { std::ranges::to(); return WriteDeleteManifests(entries, spec); } + std::unordered_map SummaryForTest() { return Summary(); } + Status CleanupForTest() { return CleanUncommitted({}); } int64_t GeneratedSnapshotId() { return SnapshotId(); } void SetDataSeqNumber(int64_t seq) { SetNewDataFilesDataSequenceNumber(seq); } void SetCaseSensitive(bool case_sensitive) { CaseSensitive(case_sensitive); } @@ -234,6 +236,14 @@ class TestMergeAppend : public MergingSnapshotUpdate { parent, std::move(io)); } + Status ValidateStagedDVsForTest(const TableMetadata& metadata, + int64_t starting_snapshot_id, + const std::shared_ptr& parent, + std::shared_ptr io) const { + return MergingSnapshotUpdate::ValidateAddedDVs( + metadata, starting_snapshot_id, Expressions::AlwaysTrue(), parent, std::move(io)); + } + bool HasDataFiles() const { return AddsDataFiles(); } bool HasDeleteFiles() const { return AddsDeleteFiles(); } bool HasDataDeletes() const { return DeletesDataFiles(); } @@ -506,9 +516,12 @@ class MergingSnapshotUpdateTest : public MinimalUpdateTestBase { std::to_string(snapshot_id) + ".avro"; ICEBERG_ASSIGN_OR_RAISE( auto writer, - ManifestListWriter::MakeWriter(table_->metadata()->format_version, snapshot_id, - parent_snapshot_id, manifest_list_path, file_io_, - sequence_number)); + ManifestListWriter::MakeWriter( + table_->metadata()->format_version, snapshot_id, parent_snapshot_id, + manifest_list_path, file_io_, sequence_number, + table_->metadata()->format_version >= 3 + ? std::make_optional(table_->metadata()->next_row_id) + : std::nullopt)); ICEBERG_RETURN_UNEXPECTED(writer->AddAll(manifests)); ICEBERG_RETURN_UNEXPECTED(writer->Close()); @@ -1265,7 +1278,8 @@ TEST_F(MergingSnapshotUpdateTest, ApplyMergesDuplicateDeletionVectors) { EXPECT_TRUE(merged_positions.IsDeleted(3)); EXPECT_FALSE(merged_positions.IsDeleted(0)); ASSERT_TRUE(merged_it->sequence_number.has_value()); - EXPECT_EQ(*merged_it->sequence_number, 7); + // Java DVUtil.writeDVs creates a new file that inherits the commit sequence. + EXPECT_EQ(*merged_it->sequence_number, kUnassignedSequenceNumber); auto single_it = std::ranges::find_if(entries, [&](const ManifestEntry& entry) { return entry.data_file->referenced_data_file == file_b_->file_path; @@ -1345,7 +1359,7 @@ TEST_F(MergingSnapshotUpdateTest, AddManifestPrimitiveAllowsExistingFilesCount) ManifestFile manifest; manifest.manifest_path = table_location_ + "/metadata/existing.avro"; manifest.content = ManifestContent::kData; - manifest.added_snapshot_id = kInvalidSnapshotId; + manifest.added_snapshot_id = std::nullopt; manifest.existing_files_count = 1; ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); @@ -1356,7 +1370,7 @@ TEST_F(MergingSnapshotUpdateTest, AddManifestPrimitiveAllowsDeletedFilesCount) { ManifestFile manifest; manifest.manifest_path = table_location_ + "/metadata/deleted.avro"; manifest.content = ManifestContent::kData; - manifest.added_snapshot_id = kInvalidSnapshotId; + manifest.added_snapshot_id = std::nullopt; manifest.deleted_files_count = 1; ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); @@ -1683,6 +1697,131 @@ TEST_F(MergingSnapshotUpdateTest, MissingRequestedDeleteDoesNotAffectSummary) { EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kTotalDataFiles), "2"); } +TEST_F(MergingSnapshotUpdateTest, RepeatedApplyPreservesCopiedAppendSummary) { + ICEBERG_UNWRAP_OR_FAIL( + auto manifest, + WriteManifest(table_location_ + "/metadata/copy-summary.avro", {file_a_})); + manifest.added_snapshot_id = 12345; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AppendManifest(manifest), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first, op->ApplyForTest(*table_->metadata(), nullptr)); + ASSERT_EQ(first.size(), 1U); + EXPECT_EQ(op->SummaryForTest().at(SnapshotSummaryFields::kAddedDataFiles), "1"); + ICEBERG_UNWRAP_OR_FAIL(auto second, op->ApplyForTest(*table_->metadata(), nullptr)); + ASSERT_EQ(second.size(), 1U); + EXPECT_EQ(second[0].manifest_path, first[0].manifest_path); + EXPECT_EQ(op->SummaryForTest().at(SnapshotSummaryFields::kAddedDataFiles), "1"); + EXPECT_EQ(op->SummaryForTest().at(SnapshotSummaryFields::kAddedRecords), "100"); +} + +TEST_F(MergingSnapshotUpdateTest, AppendAfterCleanupRecreatesAllCopies) { + ICEBERG_UNWRAP_OR_FAIL( + auto first, + WriteManifest(table_location_ + "/metadata/copy-first.avro", {file_a_})); + ICEBERG_UNWRAP_OR_FAIL( + auto second, + WriteManifest(table_location_ + "/metadata/copy-second.avro", {file_b_})); + first.added_snapshot_id = 12345; + second.added_snapshot_id = 12345; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AppendManifest(first), IsOk()); + ASSERT_THAT(op->CleanupForTest(), IsOk()); + ASSERT_THAT(op->AppendManifest(second), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, op->ApplyForTest(*table_->metadata(), nullptr)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(manifests, *table_->metadata())); + ASSERT_EQ(entries.size(), 2U); + EXPECT_EQ(op->SummaryForTest().at(SnapshotSummaryFields::kAddedDataFiles), "2"); + EXPECT_EQ(op->SummaryForTest().at(SnapshotSummaryFields::kAddedRecords), "200"); +} + +TEST_F(MergingSnapshotUpdateTest, AssignedManifestIsCopiedWhenRegistered) { + ManifestFile missing; + missing.manifest_path = table_location_ + "/metadata/missing.avro"; + missing.partition_spec_id = spec_->spec_id(); + missing.added_snapshot_id = 12345; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + EXPECT_THAT(op->AppendManifest(missing), IsError(ErrorKind::kIOError)); +} + +TEST_F(MergingSnapshotUpdateTest, CannotMergeDVsMismatchedSequenceNumbers) { + SetTableFormatVersion(3); + auto dv1 = MakeDeletionVector("/delete/dv-1.puffin", file_a_, 1); + auto dv2 = MakeDeletionVector("/delete/dv-2.puffin", file_a_, 1); + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AddDelete(dv1, 1), IsOk()); + ASSERT_THAT(op->AddDelete(dv2, 2), IsOk()); + EXPECT_THAT(op->ApplyForTest(*table_->metadata(), nullptr), + HasErrorMessage("Cannot merge DVs, mismatched sequence numbers")); +} + +TEST_F(MergingSnapshotUpdateTest, CannotMergeDVsMismatchedPartitionTuples) { + SetTableFormatVersion(3); + auto dv1 = MakeDeletionVector("/delete/dv-1.puffin", file_a_, 1); + auto dv2 = MakeDeletionVector("/delete/dv-2.puffin", file_a_, 1); + dv2->partition = file_b_->partition; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AddDelete(dv1), IsOk()); + ASSERT_THAT(op->AddDelete(dv2), IsOk()); + EXPECT_THAT(op->ApplyForTest(*table_->metadata(), nullptr), + HasErrorMessage("Cannot merge DVs, mismatched partition tuples")); +} + +TEST_F(MergingSnapshotUpdateTest, ManifestManagersUseConfiguredExecutor) { + ICEBERG_UNWRAP_OR_FAIL(auto properties, table_->NewUpdateProperties()); + properties->Set(TableProperties::kManifestMinMergeCount.key(), "2"); + ASSERT_THAT(properties->Commit(), IsOk()); + CommitFileA(); + test::ThreadExecutor executor; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + op->ScanManifestsWith(executor); + ASSERT_THAT(op->RemoveDataFile(file_a_), IsOk()); + ASSERT_THAT(op->AddFile(file_b_), IsOk()); + ASSERT_THAT(op->Commit(), IsOk()); + EXPECT_GE(executor.submit_count(), 2); + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kTotalDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); +} + +TEST_F(MergingSnapshotUpdateTest, DuplicateDVsReadWithConfiguredExecutor) { + SetTableFormatVersion(3); + ICEBERG_UNWRAP_OR_FAIL( + auto dv1, WriteDeletionVector(table_location_ + "/data/dv-1.puffin", file_a_, {1})); + ICEBERG_UNWRAP_OR_FAIL( + auto dv2, WriteDeletionVector(table_location_ + "/data/dv-2.puffin", file_a_, {2})); + test::ThreadExecutor executor; + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + op->ScanManifestsWith(executor); + ASSERT_THAT(op->AddDelete(dv1), IsOk()); + ASSERT_THAT(op->AddDelete(dv2), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto manifests, op->ApplyForTest(*table_->metadata(), nullptr)); + EXPECT_GE(executor.submit_count(), 2); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(manifests, *table_->metadata())); + ASSERT_EQ(entries.size(), 1U); + EXPECT_EQ(entries[0].data_file->record_count, 2); +} + +TEST_F(MergingSnapshotUpdateTest, DVAttemptAdvancesWithoutDuplicateWrites) { + SetTableFormatVersion(3); + ICEBERG_UNWRAP_OR_FAIL( + auto dv1, + WriteDeletionVector(table_location_ + "/data/attempt-1.puffin", file_a_, {1})); + ICEBERG_UNWRAP_OR_FAIL( + auto dv2, + WriteDeletionVector(table_location_ + "/data/attempt-2.puffin", file_a_, {2})); + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AddDelete(dv1), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto first, op->ApplyForTest(*table_->metadata(), nullptr)); + ASSERT_THAT(op->AddDelete(dv2), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto second, op->ApplyForTest(*table_->metadata(), nullptr)); + ICEBERG_UNWRAP_OR_FAIL(auto entries, ReadAllEntries(second, *table_->metadata())); + ASSERT_EQ(entries.size(), 1U); + EXPECT_THAT(entries[0].data_file->file_path, ::testing::EndsWith("-2.puffin")); + EXPECT_EQ(entries[0].data_file->record_count, 2); +} + TEST_F(MergingSnapshotUpdateTest, ValidateAddedDataFilesFailsForTruncatedHistory) { auto metadata = std::make_shared(); metadata->format_version = 2; @@ -2105,6 +2244,152 @@ TEST_F(MergingSnapshotUpdateTest, IsError(ErrorKind::kValidationFailed)); } +// Java TestRewriteFiles.testRewriteOfFileWithSuccessiveDVReplacementsRejectedAsConflict, +// testRewriteOfUnrelatedFileSucceedsWithSuccessiveDVReplacements, and +// TestRowDelta.testConcurrentSuccessiveDVsReportedAsConflictNotCorruption. +TEST_F(MergingSnapshotUpdateTest, ValidateSuccessiveDVReplacementsPerSnapshot) { + UpgradeTableToV3(); + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + auto metadata = std::make_shared(*table_->metadata()); + + auto dv1 = MakeDeletionVector("/delete/dv_first.puffin", file_a_, 1); + auto dv2 = MakeDeletionVector("/delete/dv_second.puffin", file_a_, 2); + auto parent = first_snapshot; + for (const auto& dv : {dv1, dv2}) { + const int64_t snapshot_id = parent->snapshot_id + 1; + const int64_t sequence_number = parent->sequence_number + 1; + ICEBERG_UNWRAP_OR_FAIL(auto manifest, + WriteDeleteManifest(*metadata, + table_location_ + "/metadata/dv-" + + std::to_string(snapshot_id) + ".avro", + {dv}, snapshot_id, sequence_number)); + manifest.added_snapshot_id = snapshot_id; + manifest.sequence_number = sequence_number; + manifest.min_sequence_number = sequence_number; + ICEBERG_UNWRAP_OR_FAIL( + auto snapshot, + MakeSyntheticSnapshot(DataOperation::kOverwrite, snapshot_id, parent->snapshot_id, + sequence_number, {manifest})); + metadata->snapshots.push_back(snapshot); + metadata->current_snapshot_id = snapshot_id; + metadata->last_sequence_number = sequence_number; + parent = std::move(snapshot); + } + + DataFileSet replaced_files; + replaced_files.insert(file_a_); + for (bool ignore_equality_deletes : {false, true}) { + EXPECT_THAT(TestMergeAppend::ValidateNoNewDeletesForDataFilesForTest( + *metadata, first_snapshot->snapshot_id, replaced_files, parent, + file_io_, ignore_equality_deletes), + IsError(ErrorKind::kValidationFailed)); + } + EXPECT_THAT(TestMergeAppend::ValidateNoNewDeletesForDataFilesForTest( + *metadata, first_snapshot->snapshot_id, Expressions::AlwaysTrue(), + replaced_files, parent, file_io_), + IsError(ErrorKind::kValidationFailed)); + + DataFileSet unrelated_files; + unrelated_files.insert(file_b_); + EXPECT_THAT( + TestMergeAppend::ValidateNoNewDeletesForDataFilesForTest( + *metadata, first_snapshot->snapshot_id, unrelated_files, parent, file_io_), + IsOk()); + + const std::string locations = "[" + dv2->file_path + ", " + dv1->file_path + "]"; + auto conflict = TestMergeAppend::ValidateNoNewDeleteFilesForTest( + *metadata, first_snapshot->snapshot_id, Expressions::AlwaysTrue(), parent, + file_io_); + ASSERT_THAT(conflict, IsError(ErrorKind::kValidationFailed)); + EXPECT_EQ( + conflict.error().message, + "Found new conflicting delete files that can apply to records matching true: " + + locations); + PartitionSet partitions; + partitions.add(spec_->spec_id(), file_a_->partition); + EXPECT_THAT(TestMergeAppend::ValidateNoNewDeleteFilesForTest( + *metadata, first_snapshot->snapshot_id, partitions, parent, file_io_), + IsError(ErrorKind::kValidationFailed)); +} + +TEST_F(MergingSnapshotUpdateTest, ValidateAddedDVsSkipsHistoryWhenAllReferencesAreNew) { + UpgradeTableToV3(); + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto parent, table_->current_snapshot()); + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AddFile(file_b_), IsOk()); + ASSERT_THAT(op->AddDelete(MakeDeletionVector("/delete/new_b.puffin", file_b_, 1)), + IsOk()); + test::InlineExecutor executor; + op->ScanManifestsWith(executor); + // The starting snapshot is unavailable. New-file-only DVs must return before + // opening manifests or attempting to validate this history window. + EXPECT_THAT(op->ValidateStagedDVsForTest(*table_->metadata(), -123, parent, file_io_), + IsOk()); + EXPECT_EQ(executor.submit_count(), 0); + + ASSERT_THAT(op->AddDelete(MakeDeletionVector("/delete/old_a.puffin", file_a_, 1)), + IsOk()); + EXPECT_THAT(op->ValidateStagedDVsForTest(*table_->metadata(), -123, parent, file_io_), + IsError(ErrorKind::kValidationFailed)); +} + +TEST_F(MergingSnapshotUpdateTest, ValidateAddedDVsWithMixedReferencesChecksOriginalSet) { + UpgradeTableToV3(); + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); + auto metadata = std::make_shared(*table_->metadata()); + auto concurrent_dv = MakeDeletionVector("/delete/concurrent_b.puffin", file_b_, 1); + const int64_t snapshot_id = first_snapshot->snapshot_id + 1; + const int64_t sequence_number = first_snapshot->sequence_number + 1; + ICEBERG_UNWRAP_OR_FAIL( + auto manifest, + WriteDeleteManifest(*metadata, table_location_ + "/metadata/concurrent-b.avro", + {concurrent_dv}, snapshot_id, sequence_number)); + manifest.added_snapshot_id = snapshot_id; + manifest.sequence_number = sequence_number; + manifest.min_sequence_number = sequence_number; + ICEBERG_UNWRAP_OR_FAIL( + auto parent, + MakeSyntheticSnapshot(DataOperation::kOverwrite, snapshot_id, + first_snapshot->snapshot_id, sequence_number, {manifest})); + metadata->snapshots.push_back(parent); + metadata->current_snapshot_id = snapshot_id; + metadata->last_sequence_number = sequence_number; + + ICEBERG_UNWRAP_OR_FAIL(auto op, NewMergeAppend()); + ASSERT_THAT(op->AddFile(file_b_), IsOk()); + ASSERT_THAT(op->AddDelete(MakeDeletionVector("/delete/new_b.puffin", file_b_, 1)), + IsOk()); + ASSERT_THAT(op->AddDelete(MakeDeletionVector("/delete/old_a.puffin", file_a_, 1)), + IsOk()); + test::InlineExecutor executor; + op->ScanManifestsWith(executor); + // Java's unmatched set is used only for the all-new early return. The actual + // manifest scan still checks every reference in dvsByReferencedFile. + EXPECT_THAT(op->ValidateStagedDVsForTest(*metadata, first_snapshot->snapshot_id, parent, + file_io_), + IsError(ErrorKind::kValidationFailed)); + EXPECT_EQ(executor.submit_count(), 1); +} + +TEST_F(MergingSnapshotUpdateTest, EmptyRequiredFilesStillValidateHistory) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto parent, table_->current_snapshot()); + EXPECT_THAT(TestMergeAppend::ValidateDataFilesExistForTest( + *table_->metadata(), -123, {}, false, Expressions::AlwaysTrue(), parent, + file_io_), + IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(TestMergeAppend::ValidateNoNewDeletesForDataFilesForTest( + *table_->metadata(), -123, DataFileSet{}, parent, file_io_), + IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(TestMergeAppend::ValidateNoNewDeletesForDataFilesForTest( + *table_->metadata(), -123, Expressions::AlwaysTrue(), DataFileSet{}, + parent, file_io_), + IsError(ErrorKind::kValidationFailed)); +} + TEST_F(MergingSnapshotUpdateTest, ValidateAddedDVsDetectsConflict) { CommitFileA(); ICEBERG_UNWRAP_OR_FAIL(auto first_snapshot, table_->current_snapshot()); diff --git a/src/iceberg/test/partition_value_test.cc b/src/iceberg/test/partition_value_test.cc index c72a8b6b60..e8a3d897ff 100644 --- a/src/iceberg/test/partition_value_test.cc +++ b/src/iceberg/test/partition_value_test.cc @@ -17,6 +17,7 @@ * under the License. */ +#include #include #include @@ -667,4 +668,35 @@ TEST(PartitionSetTest, ConstIteration) { EXPECT_EQ(count, 2); } +TEST(PartitionSetTest, NullsAndNaNsUseJavaPartitionIdentity) { + PartitionSet partitions; + PartitionValues null_partition(Literal::Null(int64())); + EXPECT_TRUE(partitions.add(0, null_partition)); + EXPECT_FALSE(partitions.add(0, null_partition)); + EXPECT_TRUE(partitions.contains(0, null_partition)); + EXPECT_FALSE(partitions.contains(1, null_partition)); + + const double positive_nan = std::bit_cast(uint64_t{0x7ff8000000000001}); + const double negative_nan = std::bit_cast(uint64_t{0xfff8000000000002}); + PartitionValues nan_partition(Literal::Double(positive_nan)); + PartitionValues other_nan(Literal::Double(negative_nan)); + EXPECT_TRUE(partitions.add(0, nan_partition)); + EXPECT_FALSE(partitions.add(0, other_nan)); + EXPECT_TRUE(partitions.contains(0, other_nan)); + EXPECT_EQ(PartitionValuesHash{}(nan_partition), PartitionValuesHash{}(other_nan)); +} + +TEST(PartitionMapTest, SignedZerosRemainDistinctLikeJavaDoubleComparator) { + PartitionMap partitions; + PartitionValues positive_zero(Literal::Double(0.0)); + PartitionValues negative_zero(Literal::Double(-0.0)); + partitions.put(0, positive_zero, 1); + partitions.put(0, negative_zero, 2); + EXPECT_EQ(partitions.size(), 2U); + ASSERT_TRUE(partitions.get(0, positive_zero).has_value()); + ASSERT_TRUE(partitions.get(0, negative_zero).has_value()); + EXPECT_EQ(partitions.get(0, positive_zero)->get(), 1); + EXPECT_EQ(partitions.get(0, negative_zero)->get(), 2); +} + } // namespace iceberg diff --git a/src/iceberg/update/merging_snapshot_update.cc b/src/iceberg/update/merging_snapshot_update.cc index 154a27b91e..fd863fc372 100644 --- a/src/iceberg/update/merging_snapshot_update.cc +++ b/src/iceberg/update/merging_snapshot_update.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -38,6 +39,7 @@ #include "iceberg/expression/projections.h" #include "iceberg/file_io.h" #include "iceberg/location_provider.h" +#include "iceberg/logging/log_macros.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/manifest/manifest_group.h" #include "iceberg/manifest/manifest_list.h" @@ -55,6 +57,7 @@ #include "iceberg/util/macros.h" #include "iceberg/util/snapshot_util.h" #include "iceberg/util/struct_like_set.h" +#include "iceberg/util/task_group.h" namespace iceberg { @@ -82,45 +85,6 @@ struct ValidationHistoryResult { std::unordered_set snapshot_ids; }; -Result>> ValidationAncestorsBetween( - const TableMetadata& metadata, int64_t latest_snapshot_id, - std::optional starting_snapshot_id) { - ICEBERG_ASSIGN_OR_RAISE( - auto ancestors, - SnapshotUtil::AncestorsBetween(metadata, latest_snapshot_id, starting_snapshot_id)); - if (!starting_snapshot_id.has_value()) { - if (!ancestors.empty()) { - const auto& oldest_checked = ancestors.back(); - if (oldest_checked == nullptr || oldest_checked->parent_snapshot_id.has_value()) { - return ValidationFailed( - "Cannot validate history: cannot determine complete history for snapshot {}", - latest_snapshot_id); - } - } - return ancestors; - } - - if (latest_snapshot_id == starting_snapshot_id.value()) { - return ancestors; - } - if (ancestors.empty()) { - return ValidationFailed( - "Cannot validate history: starting snapshot {} is not an ancestor " - "of snapshot {}", - starting_snapshot_id.value(), latest_snapshot_id); - } - - const auto& oldest_checked = ancestors.back(); - if (oldest_checked == nullptr || !oldest_checked->parent_snapshot_id.has_value() || - oldest_checked->parent_snapshot_id.value() != starting_snapshot_id.value()) { - return ValidationFailed( - "Cannot validate history: starting snapshot {} is not an ancestor " - "of snapshot {}", - starting_snapshot_id.value(), latest_snapshot_id); - } - return ancestors; -} - Result ValidationHistory( const TableMetadata& metadata, int64_t latest_snapshot_id, std::optional starting_snapshot_id, @@ -128,7 +92,7 @@ Result ValidationHistory( const std::shared_ptr& io) { ICEBERG_ASSIGN_OR_RAISE( auto ancestors, - ValidationAncestorsBetween(metadata, latest_snapshot_id, starting_snapshot_id)); + SnapshotUtil::AncestorsBetween(metadata, latest_snapshot_id, starting_snapshot_id)); ValidationHistoryResult result; for (const auto& snapshot : ancestors) { @@ -148,6 +112,16 @@ Result ValidationHistory( } } + // The validation window must reach the requested starting snapshot, or the + // root when no starting snapshot was provided. + if (!ancestors.empty() && + ancestors.back()->parent_snapshot_id != starting_snapshot_id) { + return ValidationFailed( + "Cannot determine history between starting snapshot {} and the last known " + "ancestor {}", + starting_snapshot_id.has_value() ? std::to_string(*starting_snapshot_id) : "null", + ancestors.back()->snapshot_id); + } return result; } @@ -355,19 +329,27 @@ std::optional DataFileLocations(const std::vector& e return FormatLocations(std::move(paths)); } -std::optional DeleteFileLocations( - const std::vector>& delete_files) { - std::vector paths; - paths.reserve(delete_files.size()); - for (const auto& delete_file : delete_files) { - if (delete_file != nullptr) { - paths.push_back(delete_file->file_path); +std::vector ReferencedDeleteFileLocations( + const std::vector>& delete_indexes) { + std::vector locations; + std::unordered_set seen; + for (const auto& deletes : delete_indexes) { + for (const auto& delete_file : deletes->ReferencedDeleteFiles()) { + if (seen.insert(delete_file->file_path).second) { + locations.push_back(delete_file->file_path); + } } } - if (paths.empty()) { - return std::optional{}; + return locations; +} + +bool ContainsPositionDeletes(const std::vector>& delete_files) { + for (const auto& delete_file : delete_files) { + if (delete_file->content == DataFile::Content::kPositionDeletes) { + return true; + } } - return FormatLocations(std::move(paths)); + return false; } Status ValidateAddedDVsInManifest( @@ -412,7 +394,8 @@ MergingSnapshotUpdate::MergingSnapshotUpdate(std::string table_name, auto file_io = ctx_->table->io(); auto data_filter_manager = ManifestFilterManager::Make( ManifestContent::kData, file_io, - [this](const std::string& location) { return DeleteFile(location); }); + [this](const std::string& location) { return DeleteFile(location); }, + [this] { return plan_executor(); }); if (!data_filter_manager.has_value()) { AddError(data_filter_manager.error()); } else { @@ -421,7 +404,8 @@ MergingSnapshotUpdate::MergingSnapshotUpdate(std::string table_name, auto delete_filter_manager = ManifestFilterManager::Make( ManifestContent::kDeletes, file_io, - [this](const std::string& location) { return DeleteFile(location); }); + [this](const std::string& location) { return DeleteFile(location); }, + [this] { return plan_executor(); }); if (!delete_filter_manager.has_value()) { AddError(delete_filter_manager.error()); } else { @@ -437,7 +421,8 @@ MergingSnapshotUpdate::MergingSnapshotUpdate(std::string table_name, auto data_merge_manager = ManifestMergeManager::Make( ManifestContent::kData, target_size_bytes, min_count_to_merge, merge_enabled, file_io, [this] { return SnapshotId(); }, - [this](const std::string& location) { return DeleteFile(location); }); + [this](const std::string& location) { return DeleteFile(location); }, + [this] { return plan_executor(); }); if (!data_merge_manager.has_value()) { AddError(data_merge_manager.error()); } else { @@ -447,7 +432,8 @@ MergingSnapshotUpdate::MergingSnapshotUpdate(std::string table_name, auto delete_merge_manager = ManifestMergeManager::Make( ManifestContent::kDeletes, target_size_bytes, min_count_to_merge, merge_enabled, file_io, [this] { return SnapshotId(); }, - [this](const std::string& location) { return DeleteFile(location); }); + [this](const std::string& location) { return DeleteFile(location); }, + [this] { return plan_executor(); }); if (!delete_merge_manager.has_value()) { AddError(delete_merge_manager.error()); } else { @@ -455,9 +441,101 @@ MergingSnapshotUpdate::MergingSnapshotUpdate(std::string table_name, } } -// ------------------------------------------------------------------------- -// Primitive API -// ------------------------------------------------------------------------- +void MergingSnapshotUpdate::SetSummaryProperty(const std::string& property, + const std::string& value) { + SnapshotUpdate::SetSummaryProperty(property, value); +} + +void MergingSnapshotUpdate::CaseSensitive(bool case_sensitive) { + case_sensitive_ = case_sensitive; + data_filter_manager_->CaseSensitive(case_sensitive); + delete_filter_manager_->CaseSensitive(case_sensitive); +} + +Result> MergingSnapshotUpdate::DataSpec() const { + if (new_data_files_by_spec_.empty()) { + return InvalidArgument( + "Cannot determine partition specs: no data files have been added"); + } + if (new_data_files_by_spec_.size() > 1) { + return InvalidArgument( + "Cannot return a single partition spec: data files with different partition " + "specs have been added"); + } + return base().PartitionSpecById(new_data_files_by_spec_.begin()->first); +} + +std::vector> MergingSnapshotUpdate::AddedDataFiles() const { + std::vector> result; + for (const auto& [spec_id, files] : new_data_files_by_spec_) { + for (const auto& file : files) { + result.push_back(file); + } + } + return result; +} + +void MergingSnapshotUpdate::FailAnyDelete() { + data_filter_manager_->FailAnyDelete(); + delete_filter_manager_->FailAnyDelete(); +} + +void MergingSnapshotUpdate::FailMissingDeletePaths() { + data_filter_manager_->FailMissingDeletePaths(); + delete_filter_manager_->FailMissingDeletePaths(); +} + +Status MergingSnapshotUpdate::DeleteByRowFilter(std::shared_ptr expr) { + // If a delete file matches the row filter, it can also be removed because the rows + // it references will also be deleted. Both filter managers receive the expression. + delete_expression_ = expr; + ICEBERG_RETURN_UNEXPECTED(data_filter_manager_->DeleteByRowFilter(expr)); + return delete_filter_manager_->DeleteByRowFilter(std::move(expr)); +} + +Status MergingSnapshotUpdate::DropPartition(int32_t spec_id, PartitionValues partition) { + // Dropping data in a partition also drops all delete files in that partition. + ICEBERG_RETURN_UNEXPECTED(data_filter_manager_->DropPartition(spec_id, partition)); + ICEBERG_RETURN_UNEXPECTED( + delete_filter_manager_->DropPartition(spec_id, std::move(partition))); + return {}; +} + +Status MergingSnapshotUpdate::DeleteDataFile(std::shared_ptr file) { + if (!file) { + return InvalidArgument("Cannot delete a null data file"); + } + auto staged_file = std::make_shared(*file); + return data_filter_manager_->DeleteFile(std::move(staged_file)); +} + +Status MergingSnapshotUpdate::DeleteDeleteFile(std::shared_ptr file) { + if (!file) { + return InvalidArgument("Cannot delete a null delete file"); + } + auto staged_file = std::make_shared(*file); + return delete_filter_manager_->DeleteFile(std::move(staged_file)); +} + +Status MergingSnapshotUpdate::DeleteByPath(std::string_view path) { + return data_filter_manager_->DeleteFile(path); +} + +bool MergingSnapshotUpdate::DeletesDataFiles() const { + return data_filter_manager_->ContainsDeletes(); +} + +bool MergingSnapshotUpdate::DeletesDeleteFiles() const { + return delete_filter_manager_->ContainsDeletes(); +} + +bool MergingSnapshotUpdate::AddsDataFiles() const { + return !new_data_files_by_spec_.empty(); +} + +bool MergingSnapshotUpdate::AddsDeleteFiles() const { + return !v2_deletes_.empty() || !dvs_by_referenced_file_.empty(); +} Status MergingSnapshotUpdate::AddDataFile(std::shared_ptr file) { if (!file) { @@ -488,40 +566,6 @@ Status MergingSnapshotUpdate::AddDataFile(std::shared_ptr file) { return {}; } -Status MergingSnapshotUpdate::ValidateNewDeleteFile(const TableMetadata& metadata, - const DataFile& file) { - if (file.content == DataFile::Content::kData) { - return InvalidArgument("Expected a delete file but got a data file: {}", - file.file_path); - } - const int8_t format_version = metadata.format_version; - const bool is_dv = ContentFileUtil::IsDV(file); - switch (format_version) { - case 1: - return InvalidArgument("Deletes are supported in V2 and above"); - case 2: - // Position deletes must NOT be DVs in v2. - if (file.content == DataFile::Content::kPositionDeletes && is_dv) { - return InvalidArgument("Must not use DVs for position deletes in V2: {}", - file.file_path); - } - break; - default: - if (format_version >= 3 && - format_version <= TableMetadata::kSupportedTableFormatVersion) { - // Position deletes MUST be DVs in v3+. - if (file.content == DataFile::Content::kPositionDeletes && !is_dv) { - return InvalidArgument("Must use DVs for position deletes in V{}: {}", - format_version, file.file_path); - } - } else { - return InvalidArgument("Unsupported format version: {}", format_version); - } - break; - } - return {}; -} - Status MergingSnapshotUpdate::AddDeleteFile(std::shared_ptr file) { return AddDeleteFile(std::move(file), std::nullopt); } @@ -531,16 +575,6 @@ Status MergingSnapshotUpdate::AddDeleteFile(std::shared_ptr file, return AddDeleteFile(std::move(file), std::optional(data_sequence_number)); } -void MergingSnapshotUpdate::PendingDeleteFilesByReferencedFile::Add( - std::string referenced_file, PendingDeleteFile file) { - auto [iter, inserted] = - index_by_referenced_file_.try_emplace(referenced_file, entries_.size()); - if (inserted) { - entries_.push_back(Entry{.referenced_file = std::move(referenced_file), .files = {}}); - } - entries_[iter->second].files.push_back(std::move(file)); -} - Status MergingSnapshotUpdate::AddDeleteFile(std::shared_ptr file, std::optional data_sequence_number) { if (!file) { @@ -568,495 +602,519 @@ Status MergingSnapshotUpdate::AddDeleteFile(std::shared_ptr file, return {}; } -Status MergingSnapshotUpdate::DeleteDataFile(std::shared_ptr file) { - if (!file) { - return InvalidArgument("Cannot delete a null data file"); +Status MergingSnapshotUpdate::ValidateNewDeleteFile(const TableMetadata& metadata, + const DataFile& file) { + if (file.content == DataFile::Content::kData) { + return InvalidArgument("Expected a delete file but got a data file: {}", + file.file_path); } - auto staged_file = std::make_shared(*file); - return data_filter_manager_->DeleteFile(std::move(staged_file)); + const int8_t format_version = metadata.format_version; + const bool is_dv = ContentFileUtil::IsDV(file); + switch (format_version) { + case 1: + return InvalidArgument("Deletes are supported in V2 and above"); + case 2: + // Position deletes must NOT be DVs in v2. + if (file.content == DataFile::Content::kPositionDeletes && is_dv) { + return InvalidArgument("Must not use DVs for position deletes in V2: {}", + file.file_path); + } + break; + default: + if (format_version >= 3 && + format_version <= TableMetadata::kSupportedTableFormatVersion) { + // Position deletes MUST be DVs in v3+. + if (file.content == DataFile::Content::kPositionDeletes && !is_dv) { + return InvalidArgument("Must use DVs for position deletes in V{}: {}", + format_version, file.file_path); + } + } else { + return InvalidArgument("Unsupported format version: {}", format_version); + } + break; + } + return {}; } -Status MergingSnapshotUpdate::DeleteDeleteFile(std::shared_ptr file) { - if (!file) { - return InvalidArgument("Cannot delete a null delete file"); +Status MergingSnapshotUpdate::AddManifest(ManifestFile manifest) { + if (manifest.content != ManifestContent::kData) { + return InvalidArgument("Cannot append delete manifest: {}", manifest.manifest_path); } - auto staged_file = std::make_shared(*file); - return delete_filter_manager_->DeleteFile(std::move(staged_file)); + if (can_inherit_snapshot_id() && !manifest.added_snapshot_id.has_value()) { + if (manifest.first_row_id.has_value()) { + return InvalidArgument("Cannot append manifest with assigned first row ID: {}", + manifest.manifest_path); + } + appended_manifests_summary_.AddedManifest(manifest); + append_manifests_.push_back(std::move(manifest)); + } else { + ICEBERG_RETURN_UNEXPECTED(EnsureRewrittenAppendManifests()); + // The manifest must be rewritten with this update's snapshot ID and null + // first_row_ids. + ICEBERG_ASSIGN_OR_RAISE(auto copied_manifest, CopyManifest(manifest)); + rewritten_append_manifests_.push_back(std::move(copied_manifest)); + append_manifests_to_copy_.push_back(std::move(manifest)); + } + return {}; } -Status MergingSnapshotUpdate::DeleteByPath(std::string_view path) { - return data_filter_manager_->DeleteFile(path); +Result MergingSnapshotUpdate::CopyManifest(const ManifestFile& manifest) { + const TableMetadata& current = base(); + ICEBERG_ASSIGN_OR_RAISE(auto schema, SnapshotUtil::SchemaFor(current, target_branch())); + ICEBERG_ASSIGN_OR_RAISE(auto spec, + current.PartitionSpecById(manifest.partition_spec_id)); + std::string path = ManifestPath(); + SnapshotSummaryBuilder summary; + ICEBERG_ASSIGN_OR_RAISE( + auto copied_manifest, + CopyAppendManifest(manifest, ctx_->table->io(), schema, spec, SnapshotId(), path, + current.format_version, &summary)); + appended_manifests_summary_.Merge(summary); + return copied_manifest; } -Status MergingSnapshotUpdate::DeleteByRowFilter(std::shared_ptr expr) { - // If a delete file matches the row filter, it can also be removed because the rows - // it references will also be deleted. Both filter managers receive the expression. - delete_expression_ = expr; - ICEBERG_RETURN_UNEXPECTED(data_filter_manager_->DeleteByRowFilter(expr)); - return delete_filter_manager_->DeleteByRowFilter(std::move(expr)); +Status MergingSnapshotUpdate::ValidateAddedDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr data_filter, const std::shared_ptr& parent, + std::shared_ptr io, bool case_sensitive) { + ICEBERG_ASSIGN_OR_RAISE( + auto conflict_entries, + MatchingAddedDataFiles(metadata, starting_snapshot_id, data_filter, + /*partition_set=*/nullptr, parent, io, case_sensitive)); + auto conflict_paths = DataFileLocations(conflict_entries); + if (conflict_paths.has_value()) { + return ValidationFailed( + "Found conflicting files that can contain records matching {}: {}", + data_filter != nullptr ? data_filter->ToString() : "any expression", + conflict_paths.value()); + } + return {}; } -Status MergingSnapshotUpdate::DropPartition(int32_t spec_id, PartitionValues partition) { - // Dropping data in a partition also drops all delete files in that partition. - ICEBERG_RETURN_UNEXPECTED(data_filter_manager_->DropPartition(spec_id, partition)); - ICEBERG_RETURN_UNEXPECTED( - delete_filter_manager_->DropPartition(spec_id, std::move(partition))); +Status MergingSnapshotUpdate::ValidateAddedDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + const PartitionSet& partition_set, const std::shared_ptr& parent, + std::shared_ptr io) { + ICEBERG_ASSIGN_OR_RAISE( + auto conflict_entries, + MatchingAddedDataFiles(metadata, starting_snapshot_id, + /*data_filter=*/nullptr, &partition_set, parent, io, + /*case_sensitive=*/true)); + auto conflict_paths = DataFileLocations(conflict_entries); + if (conflict_paths.has_value()) { + return ValidationFailed( + "Found conflicting files that can contain records matching validated " + "partitions: {}", + conflict_paths.value()); + } return {}; } -void MergingSnapshotUpdate::FailMissingDeletePaths() { - data_filter_manager_->FailMissingDeletePaths(); - delete_filter_manager_->FailMissingDeletePaths(); -} +Status MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + const DataFileSet& replaced_files, const std::shared_ptr& parent, + std::shared_ptr io, bool ignore_equality_deletes) { + if (parent == nullptr || metadata.format_version < 2) { + return {}; + } -void MergingSnapshotUpdate::FailAnyDelete() { - data_filter_manager_->FailAnyDelete(); - delete_filter_manager_->FailAnyDelete(); -} + ICEBERG_ASSIGN_OR_RAISE( + auto delete_indexes, + AddedDeleteFilesIndexedPerSnapshot(metadata, starting_snapshot_id, + /*data_filter=*/nullptr, + /*partition_set=*/nullptr, parent, io)); + ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, + StartingSequenceNumber(metadata, starting_snapshot_id)); -void MergingSnapshotUpdate::SetNewDataFilesDataSequenceNumber(int64_t sequence_number) { - new_data_files_data_seq_number_ = sequence_number; + for (const auto& data_file : replaced_files) { + for (const auto& deletes : delete_indexes) { + // If any delete applies to files written in or before the starting snapshot, fail. + ICEBERG_ASSIGN_OR_RAISE(auto delete_files, + deletes->ForDataFile(starting_sequence_number, *data_file)); + if (ignore_equality_deletes) { + if (ContainsPositionDeletes(delete_files)) { + return ValidationFailed( + "Cannot commit, found new position delete for replaced data file: {}", + data_file->file_path); + } + } else if (!delete_files.empty()) { + return ValidationFailed( + "Cannot commit, found new delete for replaced data file: {}", + data_file->file_path); + } + } + } + return {}; } -void MergingSnapshotUpdate::CaseSensitive(bool case_sensitive) { - case_sensitive_ = case_sensitive; - data_filter_manager_->CaseSensitive(case_sensitive); - delete_filter_manager_->CaseSensitive(case_sensitive); -} +Status MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr data_filter, const DataFileSet& replaced_files, + const std::shared_ptr& parent, std::shared_ptr io, + bool case_sensitive) { + if (parent == nullptr || metadata.format_version < 2) { + return {}; + } -void MergingSnapshotUpdate::SetSummaryProperty(const std::string& property, - const std::string& value) { - custom_summary_properties_[property] = value; - SnapshotUpdate::SetSummaryProperty(property, value); -} + ICEBERG_ASSIGN_OR_RAISE(auto delete_indexes, + AddedDeleteFilesIndexedPerSnapshot( + metadata, starting_snapshot_id, std::move(data_filter), + /*partition_set=*/nullptr, parent, io, case_sensitive)); -Result> MergingSnapshotUpdate::DataSpec() const { - if (new_data_files_by_spec_.empty()) { - return InvalidArgument( - "Cannot determine partition specs: no data files have been added"); - } - if (new_data_files_by_spec_.size() > 1) { - return InvalidArgument( - "Cannot return a single partition spec: data files with different partition " - "specs have been added"); - } - return base().PartitionSpecById(new_data_files_by_spec_.begin()->first); -} + ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, + StartingSequenceNumber(metadata, starting_snapshot_id)); -std::vector> MergingSnapshotUpdate::AddedDataFiles() const { - std::vector> result; - for (const auto& [spec_id, files] : new_data_files_by_spec_) { - for (const auto& file : files) { - result.push_back(file); + for (const auto& data_file : replaced_files) { + for (const auto& deletes : delete_indexes) { + ICEBERG_ASSIGN_OR_RAISE(auto delete_files, + deletes->ForDataFile(starting_sequence_number, *data_file)); + if (!delete_files.empty()) { + return ValidationFailed( + "Cannot commit, found new delete for replaced data file: {}", + data_file->file_path); + } } } - return result; + return {}; } -Status MergingSnapshotUpdate::AddManifest(ManifestFile manifest) { - if (manifest.content != ManifestContent::kData) { - return InvalidArgument("Cannot append delete manifest: {}", manifest.manifest_path); - } - if (can_inherit_snapshot_id() && !manifest.added_snapshot_id.has_value()) { - if (manifest.first_row_id.has_value()) { - return InvalidArgument("Cannot append manifest with assigned first row ID: {}", - manifest.manifest_path); - } - append_manifests_.push_back(std::move(manifest)); - } else { - append_manifests_to_copy_.push_back(std::move(manifest)); +Status MergingSnapshotUpdate::ValidateNoNewDeleteFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr data_filter, const std::shared_ptr& parent, + std::shared_ptr io, bool case_sensitive) { + std::string data_filter_text = + data_filter != nullptr ? data_filter->ToString() : "any expression"; + ICEBERG_ASSIGN_OR_RAISE(auto delete_indexes, + AddedDeleteFilesIndexedPerSnapshot( + metadata, starting_snapshot_id, std::move(data_filter), + /*partition_set=*/nullptr, parent, io, case_sensitive)); + auto locations = ReferencedDeleteFileLocations(delete_indexes); + if (!locations.empty()) { + return ValidationFailed( + "Found new conflicting delete files that can apply to records matching {}: {}", + data_filter_text, FormatLocations(std::move(locations))); } return {}; } -Result MergingSnapshotUpdate::CopyManifest(const ManifestFile& manifest) { - const TableMetadata& current = base(); - ICEBERG_ASSIGN_OR_RAISE(auto schema, SnapshotUtil::SchemaFor(current, target_branch())); - ICEBERG_ASSIGN_OR_RAISE(auto spec, - current.PartitionSpecById(manifest.partition_spec_id)); - std::string path = ManifestPath(); - return CopyAppendManifest(manifest, ctx_->table->io(), schema, spec, SnapshotId(), path, - current.format_version, &appended_manifests_summary_); +Status MergingSnapshotUpdate::ValidateNoNewDeleteFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + const PartitionSet& partition_set, const std::shared_ptr& parent, + std::shared_ptr io) { + ICEBERG_ASSIGN_OR_RAISE(auto delete_indexes, + AddedDeleteFilesIndexedPerSnapshot( + metadata, starting_snapshot_id, + /*data_filter=*/nullptr, + std::make_shared(partition_set), parent, io)); + auto locations = ReferencedDeleteFileLocations(delete_indexes); + if (!locations.empty()) { + return ValidationFailed( + "Found new conflicting delete files that can apply to records matching " + "validated partitions: {}", + FormatLocations(std::move(locations))); + } + return {}; } -// ------------------------------------------------------------------------- -// State queries -// ------------------------------------------------------------------------- +Result>> +MergingSnapshotUpdate::AddedDeleteFilesIndexedPerSnapshot( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr data_filter, std::shared_ptr partition_set, + const std::shared_ptr& parent, std::shared_ptr io, + bool case_sensitive) { + // If there is no current table state, no delete files have been added. + if (parent == nullptr || metadata.format_version < 2) { + return std::vector>{}; + } -bool MergingSnapshotUpdate::AddsDataFiles() const { - return !new_data_files_by_spec_.empty(); -} + ICEBERG_ASSIGN_OR_RAISE( + auto history, ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, + kValidateAddedDeleteFilesOperations, + ManifestContent::kDeletes, io)); -bool MergingSnapshotUpdate::AddsDeleteFiles() const { - return !v2_deletes_.empty() || !dvs_by_referenced_file_.empty(); -} + // History collects a manifest only from the snapshot that added it. Grouping by + // snapshot ID assigns each manifest to exactly one index and still reads it once. + // Preserve history order to keep the failure message deterministic. + std::vector> delete_manifests_by_snapshot; + std::unordered_map group_by_snapshot_id; + for (auto& manifest : history.manifests) { + auto [iter, inserted] = group_by_snapshot_id.try_emplace( + manifest.added_snapshot_id.value(), delete_manifests_by_snapshot.size()); + if (inserted) { + delete_manifests_by_snapshot.emplace_back(); + } + delete_manifests_by_snapshot[iter->second].push_back(std::move(manifest)); + } -bool MergingSnapshotUpdate::DeletesDataFiles() const { - return data_filter_manager_->ContainsDeletes(); + ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, + StartingSequenceNumber(metadata, starting_snapshot_id)); + std::vector> delete_indexes; + delete_indexes.reserve(delete_manifests_by_snapshot.size()); + for (auto& delete_manifests : delete_manifests_by_snapshot) { + ICEBERG_ASSIGN_OR_RAISE( + auto deletes, BuildDeleteFileIndex(metadata, io, std::move(delete_manifests), + starting_sequence_number, data_filter, + partition_set, case_sensitive)); + delete_indexes.push_back(std::move(deletes)); + } + return delete_indexes; } -bool MergingSnapshotUpdate::DeletesDeleteFiles() const { - return delete_filter_manager_->ContainsDeletes(); +Status MergingSnapshotUpdate::ValidateDeletedDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr data_filter, const std::shared_ptr& parent, + std::shared_ptr io, bool case_sensitive) { + ICEBERG_ASSIGN_OR_RAISE( + auto conflict_entries, + MatchingDeletedDataFiles(metadata, starting_snapshot_id, data_filter, + /*partition_set=*/nullptr, parent, io, case_sensitive)); + auto conflict_paths = DataFileLocations(conflict_entries); + if (conflict_paths.has_value()) { + return ValidationFailed( + "Found conflicting deleted files that can contain records matching {}: {}", + data_filter != nullptr ? data_filter->ToString() : "any expression", + conflict_paths.value()); + } + return {}; } -Status MergingSnapshotUpdate::ManagersReady() const { - ICEBERG_CHECK(data_filter_manager_ != nullptr, - "Data filter manager is not initialized"); - ICEBERG_CHECK(delete_filter_manager_ != nullptr, - "Delete filter manager is not initialized"); - ICEBERG_CHECK(data_merge_manager_ != nullptr, "Data merge manager is not initialized"); - ICEBERG_CHECK(delete_merge_manager_ != nullptr, - "Delete merge manager is not initialized"); +Status MergingSnapshotUpdate::ValidateDeletedDataFiles( + const TableMetadata& metadata, std::optional starting_snapshot_id, + const PartitionSet& partition_set, const std::shared_ptr& parent, + std::shared_ptr io) { + ICEBERG_ASSIGN_OR_RAISE( + auto conflict_entries, + MatchingDeletedDataFiles(metadata, starting_snapshot_id, + /*data_filter=*/nullptr, &partition_set, parent, io, + /*case_sensitive=*/true)); + auto conflict_paths = DataFileLocations(conflict_entries); + if (conflict_paths.has_value()) { + return ValidationFailed( + "Found conflicting deleted files that can apply to records matching " + "validated partitions: {}", + conflict_paths.value()); + } return {}; } -// ------------------------------------------------------------------------- -// Apply pipeline -// ------------------------------------------------------------------------- +void MergingSnapshotUpdate::SetNewDataFilesDataSequenceNumber(int64_t sequence_number) { + new_data_files_data_seq_number_ = sequence_number; +} -ManifestWriterFactory MergingSnapshotUpdate::MakeWriterFactory( - const std::shared_ptr& schema) { - return - [this, schema](int32_t spec_id, - ManifestContent content) -> Result> { - const TableMetadata& meta = base(); - ICEBERG_ASSIGN_OR_RAISE(auto spec, meta.PartitionSpecById(spec_id)); - std::string path = ManifestPath(); - return ManifestWriter::MakeWriter(meta.format_version, SnapshotId(), - std::move(path), ctx_->table->io(), - std::move(spec), schema, content); - }; -} - -Result> MergingSnapshotUpdate::WriteNewDataManifests() { - // If new files were staged after the cache was populated (commit retry), invalidate. - if (has_new_data_files_ && !cached_new_data_manifests_.empty()) { - for (const auto& m : cached_new_data_manifests_) { - std::ignore = DeleteFile(m.manifest_path); - } - cached_new_data_manifests_.clear(); +Status MergingSnapshotUpdate::ValidateDataFilesExist( + const TableMetadata& metadata, std::optional starting_snapshot_id, + const std::unordered_set& file_paths, bool skip_deletes, + std::shared_ptr conflict_detection_filter, + const std::shared_ptr& parent, std::shared_ptr io, + bool /*case_sensitive*/) { + if (parent == nullptr) { + return {}; } - if (!cached_new_data_manifests_.empty()) { - return cached_new_data_manifests_; - } + std::span matching_operations = + skip_deletes + ? std::span(kValidateDataFilesExistSkipDeleteOperations) + : std::span(kValidateDataFilesExistOperations); + ICEBERG_ASSIGN_OR_RAISE( + auto history, ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, + matching_operations, ManifestContent::kData, io)); - std::vector result; - for (const auto& [spec_id, data_files] : new_data_files_by_spec_) { - ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(spec_id)); - ICEBERG_ASSIGN_OR_RAISE( - auto written, - WriteDataManifests(data_files.as_span(), spec, new_data_files_data_seq_number_)); - result.insert(result.end(), std::make_move_iterator(written.begin()), - std::make_move_iterator(written.end())); + ICEBERG_ASSIGN_OR_RAISE(auto group, MakeValidationManifestGroup( + metadata, io, std::move(history.manifests))); + group->IgnoreExisting(); + group->FilterManifestEntries([&history, &file_paths](const ManifestEntry& entry) { + return entry.status != ManifestStatus::kAdded && entry.snapshot_id.has_value() && + history.snapshot_ids.contains(entry.snapshot_id.value()) && + entry.data_file != nullptr && file_paths.contains(entry.data_file->file_path); + }); + if (conflict_detection_filter != nullptr) { + group->FilterData(std::move(conflict_detection_filter)); } - cached_new_data_manifests_ = result; - has_new_data_files_ = false; - return result; + ICEBERG_ASSIGN_OR_RAISE(auto entries, group->Entries()); + auto deleted_paths = DataFileLocations(entries); + if (deleted_paths.has_value()) { + return ValidationFailed("Cannot commit, missing data files: {}", + deleted_paths.value()); + } + return {}; } -Result> -MergingSnapshotUpdate::MergeDVs() { - std::vector result; - result.reserve(dvs_by_referenced_file_.size()); - std::vector groups; - std::vector> data_sequence_numbers; - - for (const auto& entry : dvs_by_referenced_file_.entries()) { - const auto& referenced_file = entry.referenced_file; - const auto& dvs = entry.files; - if (dvs.empty()) { - continue; - } - if (dvs.size() == 1) { - result.push_back(dvs.front()); - continue; - } - - const auto& first_file = dvs.front().file; - const auto first_data_sequence_number = dvs.front().data_sequence_number; - const auto first_spec_id = first_file->partition_spec_id; - ICEBERG_PRECHECK(first_spec_id.has_value(), "DV must have a partition spec ID: {}", - first_file->file_path); +Status MergingSnapshotUpdate::ValidateAddedDVs( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr conflict_detection_filter, + const std::unordered_set& referenced_data_files, + const std::shared_ptr& parent, std::shared_ptr io, + bool case_sensitive, OptionalExecutor executor) { + if (parent == nullptr || referenced_data_files.empty()) { + return {}; + } - for (const auto& dv : dvs) { - ICEBERG_PRECHECK(dv.data_sequence_number == first_data_sequence_number, - "Cannot merge DVs, mismatched sequence numbers for {}", - referenced_file); - ICEBERG_PRECHECK(dv.file->partition_spec_id == first_spec_id, - "Cannot merge DVs, mismatched partition specs for {}", - referenced_file); - // TODO(gangwu): Java DVUtil.validateCanMerge compares partitions with - // Comparators.forType(spec.partitionType()). Use a typed C++ comparator if - // StructLikeEqual's scalar equality is not enough for future partition types. - ICEBERG_ASSIGN_OR_RAISE(auto same_partition, - StructLikeEqual(dv.file->partition, first_file->partition)); - ICEBERG_PRECHECK(same_partition, - "Cannot merge DVs, mismatched partition tuples for {}", - referenced_file); - } + ICEBERG_ASSIGN_OR_RAISE( + auto history, + ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, + kValidateAddedDVsOperations, ManifestContent::kDeletes, io)); + ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); - ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(*first_spec_id)); - DeletionVectorMergeGroup group{ - .referenced_data_file = referenced_file, - .spec = std::move(spec), - .partition = first_file->partition, - }; - group.delete_files.reserve(dvs.size()); - for (const auto& dv : dvs) { - group.delete_files.push_back(dv.file); + ICEBERG_ASSIGN_OR_RAISE(auto matching_manifests, + FilterManifestsByPartition(metadata, conflict_detection_filter, + history.manifests, case_sensitive)); + std::atomic failed{false}; + auto tasks = TaskGroup().SetExecutor(executor); + for (const auto& manifest : matching_manifests) { + if (!manifest.has_added_files()) { + continue; } - groups.push_back(std::move(group)); - data_sequence_numbers.push_back(first_data_sequence_number); - } - - if (groups.empty()) { - return result; + tasks.Submit([&, manifest]() -> Status { + if (failed.load()) { + return {}; + } + auto status = ValidateAddedDVsInManifest( + metadata, manifest, conflict_detection_filter, history.snapshot_ids, + referenced_data_files, io, schema, case_sensitive); + if (!status) { + failed.store(true); + } + return status; + }); } + return std::move(tasks).Run(); +} - ICEBERG_ASSIGN_OR_RAISE(auto location_provider, ctx_->NewLocationProvider()); - auto output_path = location_provider->NewDataLocation( - std::format("merged-dvs-{}-{}.puffin", SnapshotId(), ++dv_merge_attempt_)); - - RegisterStagedFile(output_path); - auto merged_files = DVUtil::MergeAndWriteDVs(groups, output_path, ctx_->table->io()); - if (!merged_files) { - std::ignore = DeleteFile(output_path); - return ::iceberg::unexpected(std::move(merged_files.error())); +Status MergingSnapshotUpdate::ValidateAddedDVs( + const TableMetadata& metadata, std::optional starting_snapshot_id, + std::shared_ptr conflict_filter, const std::shared_ptr& parent, + std::shared_ptr io) const { + if (parent == nullptr) { + return {}; } - std::unordered_map> merged_by_ref; - for (auto& file : merged_files.value()) { - ICEBERG_PRECHECK(file != nullptr, "Merged DV file must not be null"); - ICEBERG_PRECHECK(file->referenced_data_file.has_value(), - "Merged DV must have a referenced data file: {}", file->file_path); - const auto referenced_data_file = *file->referenced_data_file; - auto insert_result = merged_by_ref.emplace(referenced_data_file, std::move(file)); - ICEBERG_PRECHECK(insert_result.second, "Duplicate merged DV for {}", - referenced_data_file); + std::unordered_set referenced_data_files; + for (const auto& entry : dvs_by_referenced_file_.entries()) { + referenced_data_files.insert(entry.referenced_file); } - ICEBERG_PRECHECK(merged_by_ref.size() == groups.size(), - "Expected {} merged DVs but got {}", groups.size(), - merged_by_ref.size()); - - for (size_t pos = 0; pos < groups.size(); ++pos) { - auto iter = merged_by_ref.find(groups[pos].referenced_data_file); - ICEBERG_CHECK(iter != merged_by_ref.end(), "Missing merged DV for {}", - groups[pos].referenced_data_file); - PendingDeleteFile merged{.file = std::move(iter->second), - .data_sequence_number = data_sequence_numbers[pos]}; - merged_dvs_.push_back(merged); - result.push_back(std::move(merged)); + if (referenced_data_files.empty()) { + return {}; } - - // The operation-specific merged DV cache now owns cleanup for this file. - UnregisterStagedFile(output_path); - - return result; -} - -Result> MergingSnapshotUpdate::WriteNewDeleteManifests() { - // If new files were staged after the cache was populated (commit retry), invalidate. - if (has_new_delete_files_ && !cached_new_delete_manifests_.empty()) { - for (const auto& m : cached_new_delete_manifests_) { - std::ignore = DeleteFile(m.manifest_path); - } - cached_new_delete_manifests_.clear(); - std::unordered_set deleted_merged_dv_paths; - for (const auto& dv : merged_dvs_) { - if (deleted_merged_dv_paths.insert(dv.file->file_path).second) { - std::ignore = DeleteFile(dv.file->file_path); + // DVs for data files added in this operation cannot conflict with concurrent commits. + auto unmatched_referenced_data_files = referenced_data_files; + for (const auto& [spec_id, data_files] : new_data_files_by_spec_) { + for (const auto& data_file : data_files) { + if (unmatched_referenced_data_files.erase(data_file->file_path) != 0 && + unmatched_referenced_data_files.empty()) { + return {}; } } - merged_dvs_.clear(); - added_delete_files_summary_.Clear(); - } - - if (!cached_new_delete_manifests_.empty()) { - return cached_new_delete_manifests_; } - ICEBERG_ASSIGN_OR_RAISE(auto merged_dvs, MergeDVs()); - - std::vector new_delete_files; - new_delete_files.reserve(merged_dvs.size() + v2_deletes_.size()); - new_delete_files.insert(new_delete_files.end(), merged_dvs.begin(), merged_dvs.end()); - - DeleteFileSet v2_delete_set; - for (const auto& pending_file : v2_deletes_) { - if (v2_delete_set.insert(pending_file.file).second) { - new_delete_files.push_back(pending_file); - } - } + return ValidateAddedDVs(metadata, starting_snapshot_id, std::move(conflict_filter), + referenced_data_files, parent, std::move(io), case_sensitive_, + plan_executor()); +} - // Group delete files by partition spec ID, mirroring Java newDeleteFilesAsManifests(). - std::unordered_map> delete_files_by_spec; - for (const auto& pending_file : new_delete_files) { - delete_files_by_spec[pending_file.file->partition_spec_id.value()].push_back( - pending_file); - } +std::unordered_map MergingSnapshotUpdate::Summary() { + summary_builder().SetPartitionSummaryLimit( + base().properties.Get(TableProperties::kWritePartitionSummaryLimit)); + return summary_builder().Build(); +} - std::vector result; - added_delete_files_summary_.Clear(); - for (auto& [spec_id, delete_files] : delete_files_by_spec) { - ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(spec_id)); - std::vector delete_entries; - delete_entries.reserve(delete_files.size()); - for (const auto& pending_file : delete_files) { - ICEBERG_RETURN_UNEXPECTED( - added_delete_files_summary_.AddedFile(*spec, *pending_file.file)); - delete_entries.push_back(ContentFileWithSequenceNumber{ - .file = pending_file.file, - .data_sequence_number = pending_file.data_sequence_number, - }); - } - ICEBERG_ASSIGN_OR_RAISE(auto written, WriteDeleteManifests(delete_entries, spec)); - result.insert(result.end(), std::make_move_iterator(written.begin()), - std::make_move_iterator(written.end())); +Status MergingSnapshotUpdate::ValidateDeleteFilesForVersion( + const TableMetadata& current) { + // Guard buffered deletes against concurrent format upgrade. + for (const auto& file : v2_deletes_) { + ICEBERG_RETURN_UNEXPECTED(ValidateNewDeleteFile(current, *file.file)); } - - cached_new_delete_manifests_ = result; - has_new_delete_files_ = false; - return result; + return {}; } Result> MergingSnapshotUpdate::Apply( const TableMetadata& metadata_to_update, const std::shared_ptr& snapshot) { - appended_manifests_summary_.Clear(); - for (const auto& manifest : append_manifests_) { - appended_manifests_summary_.AddedManifest(manifest); - } - ICEBERG_RETURN_UNEXPECTED(ManagersReady()); - - // Re-validate buffered delete files against the current format version. A format - // upgrade between staging and commit could make previously-valid files invalid. - for (const auto& pending_file : v2_deletes_) { - ICEBERG_RETURN_UNEXPECTED( - ValidateNewDeleteFile(metadata_to_update, *pending_file.file)); - } - for (const auto& entry : dvs_by_referenced_file_.entries()) { - for (const auto& pending_file : entry.files) { - ICEBERG_RETURN_UNEXPECTED( - ValidateNewDeleteFile(metadata_to_update, *pending_file.file)); + ICEBERG_RETURN_UNEXPECTED(ValidateDeleteFilesForVersion(metadata_to_update)); + ICEBERG_ASSIGN_OR_RAISE(auto table_schema, + SnapshotUtil::SchemaFor(metadata_to_update, target_branch())); + auto writer_factory = MakeWriterFactory(table_schema); + TableMetadataCache metadata_cache(&metadata_to_update); + ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); + SnapshotReader snapshot_reader(snapshot.get()); + std::vector data_manifests; + std::vector delete_manifests; + if (snapshot) { + ICEBERG_ASSIGN_OR_RAISE(auto manifests, snapshot_reader.Manifests(ctx_->table->io())); + for (const auto& manifest : manifests) { + (manifest.content == ManifestContent::kData ? data_manifests : delete_manifests) + .push_back(&manifest); } } - ICEBERG_ASSIGN_OR_RAISE(auto target_schema, - SnapshotUtil::SchemaFor(metadata_to_update, target_branch())); - auto writer_factory = MakeWriterFactory(target_schema); - - // Step 1: Filter data manifests. - ICEBERG_ASSIGN_OR_RAISE(auto filtered_data, data_filter_manager_->FilterManifests( - target_schema, metadata_to_update, - snapshot, writer_factory)); - - // Step 2: Compute min data sequence number; set up delete filter cleanup. - // Skip unassigned manifests written in this Apply() call. - int64_t min_data_seq = metadata_to_update.last_sequence_number; - for (const auto& manifest : filtered_data) { + // Filter any existing manifests. + ICEBERG_ASSIGN_OR_RAISE(auto filtered, data_filter_manager_->FilterManifests( + table_schema, specs_by_id.get(), + data_manifests, writer_factory)); + int64_t min_data_sequence_number = metadata_to_update.last_sequence_number; + for (const auto& manifest : filtered) { + // Filter out unassigned sequence numbers in rewritten manifests. if (manifest.min_sequence_number != kUnassignedSequenceNumber) { - min_data_seq = std::min(min_data_seq, manifest.min_sequence_number); + min_data_sequence_number = + std::min(min_data_sequence_number, manifest.min_sequence_number); } } ICEBERG_RETURN_UNEXPECTED( - delete_filter_manager_->DropDeleteFilesOlderThan(min_data_seq)); - delete_filter_manager_->RemoveDanglingDeletesFor( - data_filter_manager_->FilesToBeDeleted()); + delete_filter_manager_->DropDeleteFilesOlderThan(min_data_sequence_number)); - // Step 3: Filter delete manifests. + const auto& files_to_be_deleted = data_filter_manager_->FilesToBeDeleted(); + delete_filter_manager_->RemoveDanglingDeletesFor(files_to_be_deleted); ICEBERG_ASSIGN_OR_RAISE(auto filtered_deletes, delete_filter_manager_->FilterManifests( - target_schema, metadata_to_update, - snapshot, writer_factory)); - - TableMetadataCache metadata_cache(&metadata_to_update); - ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, metadata_cache.GetPartitionSpecsById()); - ICEBERG_ASSIGN_OR_RAISE( - auto data_filter_summary, - data_filter_manager_->BuildSummary(filtered_data, specs_by_id.get())); - ICEBERG_ASSIGN_OR_RAISE( - auto delete_filter_summary, - delete_filter_manager_->BuildSummary(filtered_deletes, specs_by_id.get())); + table_schema, specs_by_id.get(), + delete_manifests, writer_factory)); - // Drop manifests with no live files - they carry no data and should not be merged - // into the new snapshot. Manifests written by the current snapshot are always kept - // regardless of live-file counts; the merge stage handles any that are empty. - int64_t snapshot_id = SnapshotId(); - auto should_keep = [snapshot_id](const ManifestFile& m) { - return m.has_added_files() || m.has_existing_files() || - m.added_snapshot_id == snapshot_id; + // Only keep manifests that have live data files or that were written by this commit. + auto should_keep = [snapshot_id = SnapshotId()](const ManifestFile& manifest) { + return manifest.has_added_files() || manifest.has_existing_files() || + manifest.added_snapshot_id == snapshot_id; }; - - // Step 4: Write (or retrieve cached) new data manifests. - ICEBERG_ASSIGN_OR_RAISE(auto written_data_manifests, WriteNewDataManifests()); - if (rewritten_append_manifests_.empty() && !append_manifests_to_copy_.empty()) { - for (const auto& manifest : append_manifests_to_copy_) { - ICEBERG_ASSIGN_OR_RAISE(auto copied, CopyManifest(manifest)); - rewritten_append_manifests_.push_back(std::move(copied)); - } - } - - // Incorporate append manifests (from AddManifest), stamping each with the - // current snapshot ID. append_manifests_ are used directly (inherit path); - // rewritten_append_manifests_ were already copied with the snapshot ID. - std::vector new_data_manifests = std::move(written_data_manifests); - for (const auto& src : append_manifests_) { - ManifestFile m = src; - m.added_snapshot_id = snapshot_id; - new_data_manifests.push_back(std::move(m)); - } - for (const auto& src : rewritten_append_manifests_) { - ManifestFile m = src; - m.added_snapshot_id = snapshot_id; - new_data_manifests.push_back(std::move(m)); - } - - // Step 5: Write (or retrieve cached) new delete manifests. - ICEBERG_ASSIGN_OR_RAISE(auto new_delete_manifests, WriteNewDeleteManifests()); - - std::erase_if(new_data_manifests, - [&](const ManifestFile& m) { return !should_keep(m); }); - std::erase_if(filtered_data, [&](const ManifestFile& m) { return !should_keep(m); }); - std::erase_if(new_delete_manifests, - [&](const ManifestFile& m) { return !should_keep(m); }); - std::erase_if(filtered_deletes, [&](const ManifestFile& m) { return !should_keep(m); }); - - // Rebuild summary from stable sub-builders so that commit retries don't double-count. + ICEBERG_ASSIGN_OR_RAISE(auto unmerged_manifests, PrepareNewDataManifests()); + unmerged_manifests.insert(unmerged_manifests.end(), filtered.begin(), filtered.end()); + std::erase_if(unmerged_manifests, + [&](const auto& manifest) { return !should_keep(manifest); }); + ICEBERG_ASSIGN_OR_RAISE(auto unmerged_delete_manifests, PrepareDeleteManifests()); + unmerged_delete_manifests.insert(unmerged_delete_manifests.end(), + filtered_deletes.begin(), filtered_deletes.end()); + std::erase_if(unmerged_delete_manifests, + [&](const auto& manifest) { return !should_keep(manifest); }); + + // Update the snapshot summary. summary_builder().Clear(); summary_builder().Merge(added_data_files_summary_); summary_builder().Merge(added_delete_files_summary_); summary_builder().Merge(appended_manifests_summary_); - for (const auto& [property, value] : custom_summary_properties_) { - summary_builder().Set(property, value); - } - summary_builder().Merge(data_filter_summary); + ICEBERG_ASSIGN_OR_RAISE(auto filter_summary, data_filter_manager_->BuildSummary( + filtered, specs_by_id.get())); + summary_builder().Merge(filter_summary); + ICEBERG_ASSIGN_OR_RAISE( + auto delete_filter_summary, + delete_filter_manager_->BuildSummary(filtered_deletes, specs_by_id.get())); summary_builder().Merge(delete_filter_summary); - // Step 6: Merge data manifests. - ICEBERG_ASSIGN_OR_RAISE(auto merged_data, data_merge_manager_->MergeManifests( - filtered_data, new_data_manifests, - metadata_to_update, writer_factory)); - - // Step 7: Merge delete manifests. - ICEBERG_ASSIGN_OR_RAISE(auto merged_deletes, delete_merge_manager_->MergeManifests( - filtered_deletes, new_delete_manifests, - metadata_to_update, writer_factory)); - - std::vector result; - result.reserve(merged_data.size() + merged_deletes.size()); - result.insert(result.end(), std::make_move_iterator(merged_data.begin()), - std::make_move_iterator(merged_data.end())); - result.insert(result.end(), std::make_move_iterator(merged_deletes.begin()), - std::make_move_iterator(merged_deletes.end())); - + ICEBERG_ASSIGN_OR_RAISE(auto manifests, + data_merge_manager_->MergeManifests( + unmerged_manifests, metadata_to_update, writer_factory)); + ICEBERG_ASSIGN_OR_RAISE( + auto merged_delete_manifests, + delete_merge_manager_->MergeManifests(unmerged_delete_manifests, metadata_to_update, + writer_factory)); + manifests.insert(manifests.end(), + std::make_move_iterator(merged_delete_manifests.begin()), + std::make_move_iterator(merged_delete_manifests.end())); + + // Replaced manifests come from the two filters and the two merge managers. + // Rewritten append manifests are new manifests (copies), not replaced ones. int32_t replaced_manifests_count = data_filter_manager_->ReplacedManifestsCount() + delete_filter_manager_->ReplacedManifestsCount() + data_merge_manager_->ReplacedManifestsCount() + delete_merge_manager_->ReplacedManifestsCount(); - summary_builder().Merge(BuildManifestCountSummary(result, replaced_manifests_count)); - - return result; + summary_builder().Merge(BuildManifestCountSummary(manifests, replaced_manifests_count)); + return manifests; } Status MergingSnapshotUpdate::CleanUncommitted( @@ -1100,326 +1158,297 @@ Status MergingSnapshotUpdate::CleanUncommittedAppends( return {}; } -Status MergingSnapshotUpdate::DeleteUncommitted( - std::vector& manifests, - const std::unordered_set& committed, bool clear) { - for (const auto& manifest : manifests) { - if (!committed.contains(manifest.manifest_path)) { - std::ignore = DeleteFile(manifest.manifest_path); - } +Result> MergingSnapshotUpdate::PrepareNewDataManifests() { + ICEBERG_RETURN_UNEXPECTED(EnsureRewrittenAppendManifests()); + std::vector new_manifests; + if (!new_data_files_by_spec_.empty()) { + ICEBERG_ASSIGN_OR_RAISE(new_manifests, NewDataFilesAsManifests()); } - if (clear) { - manifests.clear(); + new_manifests.insert(new_manifests.end(), append_manifests_.begin(), + append_manifests_.end()); + new_manifests.insert(new_manifests.end(), rewritten_append_manifests_.begin(), + rewritten_append_manifests_.end()); + for (auto& manifest : new_manifests) { + manifest.added_snapshot_id = SnapshotId(); } - return {}; -} - -std::unordered_map MergingSnapshotUpdate::Summary() { - summary_builder().SetPartitionSummaryLimit( - base().properties.Get(TableProperties::kWritePartitionSummaryLimit)); - return summary_builder().Build(); + return new_manifests; } -// ------------------------------------------------------------------------- -// Conflict-detection helpers -// ------------------------------------------------------------------------- +Result> MergingSnapshotUpdate::NewDataFilesAsManifests() { + // If new files were staged after the cache was populated (commit retry), invalidate. + if (has_new_data_files_ && !cached_new_data_manifests_.empty()) { + for (const auto& m : cached_new_data_manifests_) { + std::ignore = DeleteFile(m.manifest_path); + } + cached_new_data_manifests_.clear(); + } -Status MergingSnapshotUpdate::ValidateAddedDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, const std::shared_ptr& parent, - std::shared_ptr io, bool case_sensitive) { - ICEBERG_ASSIGN_OR_RAISE( - auto conflict_entries, - MatchingAddedDataFiles(metadata, starting_snapshot_id, data_filter, - /*partition_set=*/nullptr, parent, io, case_sensitive)); - auto conflict_paths = DataFileLocations(conflict_entries); - if (conflict_paths.has_value()) { - return ValidationFailed( - "Found conflicting files that can contain records matching {}: {}", - data_filter != nullptr ? data_filter->ToString() : "any expression", - conflict_paths.value()); + if (!cached_new_data_manifests_.empty()) { + return cached_new_data_manifests_; } - return {}; -} -Status MergingSnapshotUpdate::ValidateDataFilesExist( - const TableMetadata& metadata, std::optional starting_snapshot_id, - const std::unordered_set& file_paths, bool skip_deletes, - std::shared_ptr conflict_detection_filter, - const std::shared_ptr& parent, std::shared_ptr io, - bool /*case_sensitive*/) { - if (parent == nullptr || file_paths.empty()) { - return {}; + for (const auto& [spec_id, data_files] : new_data_files_by_spec_) { + ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(spec_id)); + ICEBERG_ASSIGN_OR_RAISE( + auto written, + WriteDataManifests(data_files.as_span(), spec, new_data_files_data_seq_number_)); + cached_new_data_manifests_.insert(cached_new_data_manifests_.end(), + std::make_move_iterator(written.begin()), + std::make_move_iterator(written.end())); } - std::span matching_operations = - skip_deletes - ? std::span(kValidateDataFilesExistSkipDeleteOperations) - : std::span(kValidateDataFilesExistOperations); - ICEBERG_ASSIGN_OR_RAISE( - auto history, ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, - matching_operations, ManifestContent::kData, io)); + has_new_data_files_ = false; + return cached_new_data_manifests_; +} - ICEBERG_ASSIGN_OR_RAISE(auto group, MakeValidationManifestGroup( - metadata, io, std::move(history.manifests))); - group->IgnoreExisting(); - group->FilterManifestEntries([&history, &file_paths](const ManifestEntry& entry) { - return entry.status != ManifestStatus::kAdded && entry.snapshot_id.has_value() && - history.snapshot_ids.contains(entry.snapshot_id.value()) && - entry.data_file != nullptr && file_paths.contains(entry.data_file->file_path); - }); - if (conflict_detection_filter != nullptr) { - group->FilterData(std::move(conflict_detection_filter)); +Result> MergingSnapshotUpdate::PrepareDeleteManifests() { + if (!AddsDeleteFiles()) { + return std::vector{}; } + return NewDeleteFilesAsManifests(); +} - ICEBERG_ASSIGN_OR_RAISE(auto entries, group->Entries()); - auto deleted_paths = DataFileLocations(entries); - if (deleted_paths.has_value()) { - return ValidationFailed("Cannot commit, missing data files: {}", - deleted_paths.value()); +Result> MergingSnapshotUpdate::NewDeleteFilesAsManifests() { + // If new files were staged after the cache was populated (commit retry), invalidate. + if (has_new_delete_files_ && !cached_new_delete_manifests_.empty()) { + for (const auto& m : cached_new_delete_manifests_) { + std::ignore = DeleteFile(m.manifest_path); + } + cached_new_delete_manifests_.clear(); + std::unordered_set deleted_merged_dv_paths; + for (const auto& dv : merged_dvs_) { + if (deleted_merged_dv_paths.insert(dv.file->file_path).second) { + std::ignore = DeleteFile(dv.file->file_path); + } + } + merged_dvs_.clear(); + added_delete_files_summary_.Clear(); } - return {}; -} -Status MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - const DataFileSet& replaced_files, const std::shared_ptr& parent, - std::shared_ptr io, bool ignore_equality_deletes) { - if (parent == nullptr || replaced_files.empty() || metadata.format_version < 2) { - return {}; + if (!cached_new_delete_manifests_.empty()) { + return cached_new_delete_manifests_; } - ICEBERG_ASSIGN_OR_RAISE(auto deletes, - AddedDeleteFiles(metadata, starting_snapshot_id, - /*data_filter=*/nullptr, - /*partition_set=*/nullptr, parent, io)); + ICEBERG_ASSIGN_OR_RAISE(auto merged_dvs, MergeDVs()); - ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, - StartingSequenceNumber(metadata, starting_snapshot_id)); + std::vector new_delete_files; + new_delete_files.reserve(merged_dvs.size() + v2_deletes_.size()); + new_delete_files.insert(new_delete_files.end(), merged_dvs.begin(), merged_dvs.end()); - for (const auto& data_file : replaced_files) { - ICEBERG_ASSIGN_OR_RAISE(auto delete_files, - deletes->ForDataFile(starting_sequence_number, *data_file)); - if (ignore_equality_deletes) { - // Only fail on position deletes — equality deletes at higher sequence numbers - // still apply to the rewritten files and are not a conflict. - for (const auto& df : delete_files) { - if (df->content == DataFile::Content::kPositionDeletes) { - return ValidationFailed( - "Cannot commit, found new position delete for replaced data file: {}", - data_file->file_path); - } - } - } else { - if (!delete_files.empty()) { - return ValidationFailed( - "Cannot commit, found new delete for replaced data file: {}", - data_file->file_path); - } + DeleteFileSet v2_delete_set; + for (const auto& pending_file : v2_deletes_) { + if (v2_delete_set.insert(pending_file.file).second) { + new_delete_files.push_back(pending_file); } } - return {}; -} -Status MergingSnapshotUpdate::ValidateAddedDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - const PartitionSet& partition_set, const std::shared_ptr& parent, - std::shared_ptr io) { - ICEBERG_ASSIGN_OR_RAISE( - auto conflict_entries, - MatchingAddedDataFiles(metadata, starting_snapshot_id, - /*data_filter=*/nullptr, &partition_set, parent, io, - /*case_sensitive=*/true)); - auto conflict_paths = DataFileLocations(conflict_entries); - if (conflict_paths.has_value()) { - return ValidationFailed( - "Found conflicting files that can contain records matching validated " - "partitions: {}", - conflict_paths.value()); + // Group delete files by partition spec ID, mirroring Java newDeleteFilesAsManifests(). + std::unordered_map> delete_files_by_spec; + for (const auto& pending_file : new_delete_files) { + delete_files_by_spec[pending_file.file->partition_spec_id.value()].push_back( + pending_file); } - return {}; + + added_delete_files_summary_.Clear(); + for (auto& [spec_id, delete_files] : delete_files_by_spec) { + ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(spec_id)); + std::vector delete_entries; + delete_entries.reserve(delete_files.size()); + for (const auto& pending_file : delete_files) { + ICEBERG_RETURN_UNEXPECTED( + added_delete_files_summary_.AddedFile(*spec, *pending_file.file)); + delete_entries.push_back(ContentFileWithSequenceNumber{ + .file = pending_file.file, + .data_sequence_number = pending_file.data_sequence_number, + }); + } + ICEBERG_ASSIGN_OR_RAISE(auto written, WriteDeleteManifests(delete_entries, spec)); + cached_new_delete_manifests_.insert(cached_new_delete_manifests_.end(), + std::make_move_iterator(written.begin()), + std::make_move_iterator(written.end())); + } + + has_new_delete_files_ = false; + return cached_new_delete_manifests_; } -Status MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, const DataFileSet& replaced_files, - const std::shared_ptr& parent, std::shared_ptr io, - bool case_sensitive) { - if (parent == nullptr || replaced_files.empty() || metadata.format_version < 2) { - return {}; +Result> +MergingSnapshotUpdate::MergeDVs() { + for (const auto& entry : dvs_by_referenced_file_.entries()) { + if (entry.files.size() > 1) { + ICEBERG_LOG_WARN("Merging {} duplicate DVs for data file {} in table {}.", + entry.files.size(), entry.referenced_file, table_name_); + } } + // Java allocates an attempt name even when no duplicate DV requires a write. + ICEBERG_ASSIGN_OR_RAISE(auto location_provider, ctx_->NewLocationProvider()); + auto output_path = location_provider->NewDataLocation( + std::format("merged-dvs-{}-{}.puffin", SnapshotId(), ++dv_merge_attempt_)); - ICEBERG_ASSIGN_OR_RAISE( - auto deletes, - AddedDeleteFiles(metadata, starting_snapshot_id, std::move(data_filter), - /*partition_set=*/nullptr, parent, io, case_sensitive)); + std::vector result; + result.reserve(dvs_by_referenced_file_.size()); + std::vector groups; - ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, - StartingSequenceNumber(metadata, starting_snapshot_id)); + for (const auto& entry : dvs_by_referenced_file_.entries()) { + const auto& referenced_file = entry.referenced_file; + const auto& dvs = entry.files; + if (dvs.empty()) { + continue; + } + if (dvs.size() == 1) { + result.push_back(dvs.front()); + continue; + } - for (const auto& data_file : replaced_files) { - ICEBERG_ASSIGN_OR_RAISE(auto delete_files, - deletes->ForDataFile(starting_sequence_number, *data_file)); - if (!delete_files.empty()) { - return ValidationFailed( - "Cannot commit, found new delete for replaced data file: {}", - data_file->file_path); + const auto& first_file = dvs.front().file; + const auto first_data_sequence_number = dvs.front().data_sequence_number; + const auto first_spec_id = first_file->partition_spec_id; + ICEBERG_PRECHECK(first_spec_id.has_value(), "DV must have a partition spec ID: {}", + first_file->file_path); + + for (const auto& dv : dvs) { + ICEBERG_PRECHECK(dv.data_sequence_number == first_data_sequence_number, + "Cannot merge DVs, mismatched sequence numbers for {}", + referenced_file); + ICEBERG_PRECHECK(dv.file->partition_spec_id == first_spec_id, + "Cannot merge DVs, mismatched partition specs for {}", + referenced_file); + // StructLikeEqual uses Java scalar equality, including nulls, canonical NaNs, + // and distinct signed zeros, matching partition comparator equality. + ICEBERG_ASSIGN_OR_RAISE(auto same_partition, + StructLikeEqual(dv.file->partition, first_file->partition)); + ICEBERG_PRECHECK(same_partition, + "Cannot merge DVs, mismatched partition tuples for {}", + referenced_file); } - } - return {}; -} -Status MergingSnapshotUpdate::ValidateNoNewDeleteFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, const std::shared_ptr& parent, - std::shared_ptr io, bool case_sensitive) { - std::string data_filter_text = - data_filter != nullptr ? data_filter->ToString() : "any expression"; - ICEBERG_ASSIGN_OR_RAISE( - auto deletes, - AddedDeleteFiles(metadata, starting_snapshot_id, std::move(data_filter), - /*partition_set=*/nullptr, parent, io, case_sensitive)); - auto referenced_delete_files = deletes->ReferencedDeleteFiles(); - auto delete_paths = DeleteFileLocations(referenced_delete_files); - if (delete_paths.has_value()) { - return ValidationFailed( - "Found new conflicting delete files that can apply to records matching {}: {}", - data_filter_text, delete_paths.value()); + ICEBERG_ASSIGN_OR_RAISE(auto spec, base().PartitionSpecById(*first_spec_id)); + DeletionVectorMergeGroup group{ + .referenced_data_file = referenced_file, + .spec = std::move(spec), + .partition = first_file->partition, + }; + group.delete_files.reserve(dvs.size()); + for (const auto& dv : dvs) { + group.delete_files.push_back(dv.file); + } + groups.push_back(std::move(group)); } - return {}; -} -Status MergingSnapshotUpdate::ValidateNoNewDeleteFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - const PartitionSet& partition_set, const std::shared_ptr& parent, - std::shared_ptr io) { - ICEBERG_ASSIGN_OR_RAISE( - auto deletes, - AddedDeleteFiles(metadata, starting_snapshot_id, - /*data_filter=*/nullptr, - std::make_shared(partition_set), parent, io)); - auto referenced_delete_files = deletes->ReferencedDeleteFiles(); - auto delete_paths = DeleteFileLocations(referenced_delete_files); - if (delete_paths.has_value()) { - return ValidationFailed( - "Found new conflicting delete files that can apply to records matching " - "validated partitions: {}", - delete_paths.value()); + if (groups.empty()) { + return result; } - return {}; -} -Status MergingSnapshotUpdate::ValidateDeletedDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, const std::shared_ptr& parent, - std::shared_ptr io, bool case_sensitive) { - ICEBERG_ASSIGN_OR_RAISE( - auto conflict_entries, - MatchingDeletedDataFiles(metadata, starting_snapshot_id, data_filter, - /*partition_set=*/nullptr, parent, io, case_sensitive)); - auto conflict_paths = DataFileLocations(conflict_entries); - if (conflict_paths.has_value()) { - return ValidationFailed( - "Found conflicting deleted files that can contain records matching {}: {}", - data_filter != nullptr ? data_filter->ToString() : "any expression", - conflict_paths.value()); + RegisterStagedFile(output_path); + auto merged_files = + DVUtil::MergeAndWriteDVs(groups, output_path, ctx_->table->io(), plan_executor()); + if (!merged_files) { + std::ignore = DeleteFile(output_path); + return ::iceberg::unexpected(std::move(merged_files.error())); } - return {}; -} -Status MergingSnapshotUpdate::ValidateDeletedDataFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - const PartitionSet& partition_set, const std::shared_ptr& parent, - std::shared_ptr io) { - ICEBERG_ASSIGN_OR_RAISE( - auto conflict_entries, - MatchingDeletedDataFiles(metadata, starting_snapshot_id, - /*data_filter=*/nullptr, &partition_set, parent, io, - /*case_sensitive=*/true)); - auto conflict_paths = DataFileLocations(conflict_entries); - if (conflict_paths.has_value()) { - return ValidationFailed( - "Found conflicting deleted files that can apply to records matching " - "validated partitions: {}", - conflict_paths.value()); + std::unordered_map> merged_by_ref; + for (auto& file : merged_files.value()) { + ICEBERG_PRECHECK(file != nullptr, "Merged DV file must not be null"); + ICEBERG_PRECHECK(file->referenced_data_file.has_value(), + "Merged DV must have a referenced data file: {}", file->file_path); + const auto referenced_data_file = *file->referenced_data_file; + auto insert_result = merged_by_ref.emplace(referenced_data_file, std::move(file)); + ICEBERG_PRECHECK(insert_result.second, "Duplicate merged DV for {}", + referenced_data_file); } - return {}; -} + ICEBERG_PRECHECK(merged_by_ref.size() == groups.size(), + "Expected {} merged DVs but got {}", groups.size(), + merged_by_ref.size()); -Result> MergingSnapshotUpdate::AddedDeleteFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, std::shared_ptr partition_set, - const std::shared_ptr& parent, std::shared_ptr io, - bool case_sensitive) { - if (parent == nullptr || metadata.format_version < 2) { - ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); - ICEBERG_ASSIGN_OR_RAISE(auto specs_by_id, PartitionSpecsByIdMap(metadata)); - ICEBERG_ASSIGN_OR_RAISE( - auto builder, - DeleteFileIndex::BuilderFor(io, std::move(schema), std::move(specs_by_id), - /*delete_manifests=*/{})); - return builder.Build(); + for (size_t pos = 0; pos < groups.size(); ++pos) { + auto iter = merged_by_ref.find(groups[pos].referenced_data_file); + ICEBERG_CHECK(iter != merged_by_ref.end(), "Missing merged DV for {}", + groups[pos].referenced_data_file); + // A newly written merged DV inherits this commit's sequence number, as in + // DVUtil.writeDVs; unchanged single DVs retain their explicit sequence number. + PendingDeleteFile merged{.file = std::move(iter->second), + .data_sequence_number = std::nullopt}; + merged_dvs_.push_back(merged); + result.push_back(std::move(merged)); } - ICEBERG_ASSIGN_OR_RAISE( - auto history, ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, - kValidateAddedDeleteFilesOperations, - ManifestContent::kDeletes, io)); + // The operation-specific merged DV cache now owns cleanup for this file. + UnregisterStagedFile(output_path); - ICEBERG_ASSIGN_OR_RAISE(auto starting_sequence_number, - StartingSequenceNumber(metadata, starting_snapshot_id)); - return BuildDeleteFileIndex(metadata, io, std::move(history.manifests), - starting_sequence_number, std::move(data_filter), - std::move(partition_set), case_sensitive); + return result; } -Status MergingSnapshotUpdate::ValidateAddedDVs( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr conflict_detection_filter, - const std::unordered_set& referenced_data_files, - const std::shared_ptr& parent, std::shared_ptr io, - bool case_sensitive) { - if (parent == nullptr || referenced_data_files.empty()) { +Status MergingSnapshotUpdate::EnsureRewrittenAppendManifests() { + // C++ transactions clean failed staging attempts before replay. Recreate copies + // from their originals after that cleanup; Java retains them across commit retries. + if (rewritten_append_manifests_.size() == append_manifests_to_copy_.size()) { return {}; } - - ICEBERG_ASSIGN_OR_RAISE( - auto history, - ValidationHistory(metadata, parent->snapshot_id, starting_snapshot_id, - kValidateAddedDVsOperations, ManifestContent::kDeletes, io)); - ICEBERG_ASSIGN_OR_RAISE(auto schema, metadata.Schema()); - - ICEBERG_ASSIGN_OR_RAISE(auto matching_manifests, - FilterManifestsByPartition(metadata, conflict_detection_filter, - history.manifests, case_sensitive)); - for (const auto& manifest : matching_manifests) { - if (!manifest.has_added_files()) { - continue; + if (rewritten_append_manifests_.empty()) { + appended_manifests_summary_.Clear(); + for (const auto& manifest : append_manifests_) { + appended_manifests_summary_.AddedManifest(manifest); } - ICEBERG_RETURN_UNEXPECTED(ValidateAddedDVsInManifest( - metadata, manifest, conflict_detection_filter, history.snapshot_ids, - referenced_data_files, io, schema, case_sensitive)); + } + while (rewritten_append_manifests_.size() < append_manifests_to_copy_.size()) { + const auto& manifest = append_manifests_to_copy_[rewritten_append_manifests_.size()]; + ICEBERG_ASSIGN_OR_RAISE(auto copied_manifest, CopyManifest(manifest)); + rewritten_append_manifests_.push_back(std::move(copied_manifest)); } return {}; } -Status MergingSnapshotUpdate::ValidateAddedDVs( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr conflict_filter, const std::shared_ptr& parent, - std::shared_ptr io) const { - if (parent == nullptr) { - return {}; +Status MergingSnapshotUpdate::DeleteUncommitted( + std::vector& manifests, + const std::unordered_set& committed, bool clear) { + bool any_deleted = false; + for (const auto& manifest : manifests) { + if (!committed.contains(manifest.manifest_path)) { + std::ignore = DeleteFile(manifest.manifest_path); + any_deleted = true; + } } - - std::unordered_set referenced_data_files; - for (const auto& entry : dvs_by_referenced_file_.entries()) { - referenced_data_files.insert(entry.referenced_file); + if (clear && any_deleted) { + manifests.clear(); } - if (referenced_data_files.empty()) { - return {}; + return {}; +} + +Status MergingSnapshotUpdate::ManagersReady() const { + ICEBERG_CHECK(data_filter_manager_ != nullptr, + "Data filter manager is not initialized"); + ICEBERG_CHECK(delete_filter_manager_ != nullptr, + "Delete filter manager is not initialized"); + ICEBERG_CHECK(data_merge_manager_ != nullptr, "Data merge manager is not initialized"); + ICEBERG_CHECK(delete_merge_manager_ != nullptr, + "Delete merge manager is not initialized"); + return {}; +} + +ManifestWriterFactory MergingSnapshotUpdate::MakeWriterFactory( + const std::shared_ptr& schema) { + // Resolve lazy snapshot state before the managers invoke writers in parallel. + const int64_t snapshot_id = SnapshotId(); + return [this, schema, snapshot_id]( + int32_t spec_id, + ManifestContent content) -> Result> { + const TableMetadata& current = base(); + ICEBERG_ASSIGN_OR_RAISE(auto spec, current.PartitionSpecById(spec_id)); + return ManifestWriter::MakeWriter(current.format_version, snapshot_id, ManifestPath(), + ctx_->table->io(), std::move(spec), schema, + content); + }; +} + +void MergingSnapshotUpdate::PendingDeleteFilesByReferencedFile::Add( + std::string referenced_file, PendingDeleteFile file) { + auto [iter, inserted] = + index_by_referenced_file_.try_emplace(referenced_file, entries_.size()); + if (inserted) { + entries_.push_back(Entry{.referenced_file = std::move(referenced_file), .files = {}}); } - return ValidateAddedDVs(metadata, starting_snapshot_id, std::move(conflict_filter), - referenced_data_files, parent, std::move(io), case_sensitive_); + entries_[iter->second].files.push_back(std::move(file)); } } // namespace iceberg diff --git a/src/iceberg/update/merging_snapshot_update.h b/src/iceberg/update/merging_snapshot_update.h index e1cd7303bd..29549fe277 100644 --- a/src/iceberg/update/merging_snapshot_update.h +++ b/src/iceberg/update/merging_snapshot_update.h @@ -158,7 +158,7 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { /// \brief Returns true if any delete files have been registered for removal. bool DeletesDeleteFiles() const; - /// \brief Returns the row-filter expression set via DeleteByRowFilter, or nullptr. + /// \brief Returns the row-filter expression set via DeleteByRowFilter, or AlwaysFalse. const std::shared_ptr& RowFilter() const { return delete_expression_; } /// \brief Returns the single partition spec for all staged data files. @@ -275,13 +275,18 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { const std::shared_ptr& parent, std::shared_ptr io); - /// \brief Build a DeleteFileIndex of delete files added since starting_snapshot_id. - static Result> AddedDeleteFiles( - const TableMetadata& metadata, std::optional starting_snapshot_id, - std::shared_ptr data_filter, - std::shared_ptr partition_set, - const std::shared_ptr& parent, std::shared_ptr io, - bool case_sensitive = true); + /// \brief Build one delete index per snapshot in the validation window. + /// + /// An index allows at most one DV per data file. Successive snapshots may replace + /// that DV, so callers must check every index instead of combining the window. + static Result>> + AddedDeleteFilesIndexedPerSnapshot(const TableMetadata& metadata, + std::optional starting_snapshot_id, + std::shared_ptr data_filter, + std::shared_ptr partition_set, + const std::shared_ptr& parent, + std::shared_ptr io, + bool case_sensitive = true); /// \brief Return an error if any snapshot after starting_snapshot_id, or from /// the beginning if unset, added a deletion vector that conflicts with DVs being @@ -291,7 +296,7 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { std::shared_ptr conflict_filter, const std::unordered_set& referenced_data_files, const std::shared_ptr& parent, std::shared_ptr io, - bool case_sensitive = true); + bool case_sensitive = true, OptionalExecutor executor = std::nullopt); /// \brief Return an error if a staged deletion vector conflicts with a deletion /// vector added since starting_snapshot_id. @@ -337,16 +342,21 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { std::optional data_sequence_number); Status ManagersReady() const; + Status EnsureRewrittenAppendManifests(); void SetSummaryProperty(const std::string& property, const std::string& value) override; + Status ValidateDeleteFilesForVersion(const TableMetadata& current); + + Result> PrepareNewDataManifests(); + Result> PrepareDeleteManifests(); Result> MergeDVs(); /// \brief Write new data manifests for staged data files; caches the result. - Result> WriteNewDataManifests(); + Result> NewDataFilesAsManifests(); /// \brief Write new delete manifests for staged delete files; caches the result. - Result> WriteNewDeleteManifests(); + Result> NewDeleteFilesAsManifests(); Status CleanUncommittedAppends(const std::unordered_set& committed); @@ -363,7 +373,6 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { SnapshotSummaryBuilder added_data_files_summary_; SnapshotSummaryBuilder added_delete_files_summary_; SnapshotSummaryBuilder appended_manifests_summary_; - std::unordered_map custom_summary_properties_; std::unique_ptr data_filter_manager_; std::unique_ptr delete_filter_manager_; diff --git a/src/iceberg/util/partition_value_util.h b/src/iceberg/util/partition_value_util.h index abfc1ca873..9a00c4e8a8 100644 --- a/src/iceberg/util/partition_value_util.h +++ b/src/iceberg/util/partition_value_util.h @@ -52,7 +52,9 @@ struct PartitionValuesHash { std::size_t hash = 0; LiteralHash literal_hash; for (const auto& literal : partition.values()) { - hash ^= literal_hash(literal) + kHashPrime + (hash << 6) + (hash >> 2); + // Java partition comparison considers all NaNs equal, regardless of payload. + const size_t value_hash = literal.IsNaN() ? kHashPrime : literal_hash(literal); + hash ^= value_hash + kHashPrime + (hash << 6) + (hash >> 2); } return hash; } @@ -61,7 +63,16 @@ struct PartitionValuesHash { /// \brief Equality functor for PartitionValues. struct PartitionValuesEqual { bool operator()(const PartitionValues& lhs, const PartitionValues& rhs) const { - return lhs == rhs; + if (lhs.num_fields() != rhs.num_fields()) return false; + for (size_t pos = 0; pos < lhs.num_fields(); ++pos) { + const auto& left = lhs.values()[pos]; + const auto& right = rhs.values()[pos]; + if (left.IsNull() && right.IsNull()) continue; + if (left.IsNaN() && right.IsNaN() && left.value().index() == right.value().index()) + continue; + if (!(left == right)) return false; + } + return true; } }; @@ -89,22 +100,22 @@ struct PartitionKeyEqual { // Equality for PartitionKey vs PartitionKey bool operator()(const PartitionKey& lhs, const PartitionKey& rhs) const { - return lhs.first == rhs.first && lhs.second == rhs.second; + return lhs.first == rhs.first && PartitionValuesEqual{}(lhs.second, rhs.second); } // Equality for PartitionKey vs PartitionKeyRef (heterogeneous lookup) bool operator()(const PartitionKey& lhs, const PartitionKeyRef& rhs) const { - return lhs.first == rhs.spec_id && lhs.second == rhs.values; + return lhs.first == rhs.spec_id && PartitionValuesEqual{}(lhs.second, rhs.values); } // Equality for PartitionKeyRef vs PartitionKey (heterogeneous lookup) bool operator()(const PartitionKeyRef& lhs, const PartitionKey& rhs) const { - return lhs.spec_id == rhs.first && lhs.values == rhs.second; + return lhs.spec_id == rhs.first && PartitionValuesEqual{}(lhs.values, rhs.second); } // Equality for PartitionKeyRef vs PartitionKeyRef (heterogeneous lookup) bool operator()(const PartitionKeyRef& lhs, const PartitionKeyRef& rhs) const { - return lhs.spec_id == rhs.spec_id && lhs.values == rhs.values; + return lhs.spec_id == rhs.spec_id && PartitionValuesEqual{}(lhs.values, rhs.values); } };