From 71316548d4780ad7cca4e16d0effd79474875305 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Wed, 23 Sep 2026 18:19:10 +0200 Subject: [PATCH 01/11] Added logic to merge small files into bins --- src/Core/Settings.cpp | 10 + src/Core/SettingsChangesHistory.cpp | 2 + .../DataLakes/Iceberg/BinPackRewrite.cpp | 787 ++++++++++++++++++ .../DataLakes/Iceberg/BinPackRewrite.h | 42 + .../DataLakes/Iceberg/IcebergMetadata.cpp | 52 +- .../DataLakes/Iceberg/IcebergWrites.cpp | 11 +- .../DataLakes/Iceberg/IcebergWrites.h | 3 + .../DataLakes/Iceberg/MetadataGenerator.cpp | 115 +++ .../DataLakes/Iceberg/MetadataGenerator.h | 13 + .../Iceberg/tests/gtest_bin_pack_rewrite.cpp | 225 +++++ .../test_bin_pack_rewrite.py | 181 ++++ 11 files changed, 1420 insertions(+), 21 deletions(-) create mode 100644 src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp create mode 100644 src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h create mode 100644 src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp create mode 100644 tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index d0725772d8aa..b3ad58187dce 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -8370,6 +8370,16 @@ Allow to explicitly use 'OPTIMIZE' for iceberg tables. Minimum number of manifest files required to trigger manifest-only compaction via OPTIMIZE TABLE ... MANIFEST. If the current number of manifest files is less than or equal to this threshold, compaction is skipped. Requires allow_experimental_iceberg_compaction to be enabled. +)", EXPERIMENTAL) \ + DECLARE(UInt64, iceberg_target_data_file_size_bytes, 536870912, R"( +Target data file size in bytes for Iceberg bin-packing compaction via OPTIMIZE TABLE. +Small files are merged until the result approaches this size. Default is 512 MiB. +Requires allow_experimental_iceberg_compaction to be enabled. +)", EXPERIMENTAL) \ + DECLARE(UInt64, iceberg_min_data_file_size_bytes, 402653184, R"( +Data files smaller than this threshold are candidates for bin-packing compaction via OPTIMIZE TABLE. +Default is 384 MiB (75% of iceberg_target_data_file_size_bytes). +Requires allow_experimental_iceberg_compaction to be enabled. )", EXPERIMENTAL) \ DECLARE(Bool, allow_iceberg_remove_orphan_files, false, R"( Allow to use 'ALTER TABLE ... EXECUTE remove_orphan_files()' for iceberg tables. diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index f093c4054cf6..79870cb4498f 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -42,6 +42,8 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() addSettingsChanges(settings_changes_history, "26.6.2.20001.altinityantalya", { {"use_puffin_files_cache", false, true, "Enables cache of parsed Puffin file content such as deletion vectors."}, + {"iceberg_target_data_file_size_bytes", 536870912, 536870912, "Target file size for Iceberg bin-packing compaction (default 512 MiB)."}, + {"iceberg_min_data_file_size_bytes", 402653184, 402653184, "Files below this size are candidates for Iceberg bin-packing compaction (default 384 MiB)."}, }); addSettingsChanges(settings_changes_history, "26.6", diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp new file mode 100644 index 000000000000..fe1ea5b4c797 --- /dev/null +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -0,0 +1,787 @@ +#include + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#if USE_AVRO + +namespace DB::ErrorCodes +{ + extern const int BAD_ARGUMENTS; + extern const int LOGICAL_ERROR; + extern const int ICEBERG_SPECIFICATION_VIOLATION; +} + +namespace DB::Setting +{ + extern const SettingsUInt64 iceberg_target_data_file_size_bytes; + extern const SettingsUInt64 iceberg_min_data_file_size_bytes; +} + +namespace DB::DataLakeStorageSetting +{ + extern const DataLakeStorageSettingsBool iceberg_use_version_hint; +} + +namespace DB::Iceberg +{ + +namespace +{ + +/// A single small data file selected for bin-packing. +struct SmallFileEntry +{ + IcebergPathFromMetadata file_path; + Int64 record_count; + Int64 file_size_in_bytes; + String file_format; + Row partition_key; + std::optional sort_order_id; + /// Lineage from the source manifest entry. + std::optional snapshot_id; + std::optional sequence_number; + std::optional file_sequence_number; + /// Per-column statistics from the source manifest. + DataFileColumnStatistics column_stats; +}; + +/// A bin: a group of small files from the same partition to be merged. +struct Bin +{ + Row partition_key; + std::vector files; + Int64 total_bytes = 0; + Int64 total_records = 0; +}; + +/// Key for grouping files by partition. +struct PartitionKeyHash +{ + std::hash hasher; + size_t operator()(const Row & row) const + { + size_t result = 0; + FieldVisitorDump dump_visitor; + for (const auto & value : row) + result ^= hasher(applyVisitor(dump_visitor, value)); + return result; + } +}; + +struct PartitionKeyEqual +{ + bool operator()(const Row & a, const Row & b) const + { + if (a.size() != b.size()) + return false; + for (size_t i = 0; i < a.size(); ++i) + if (a[i] != b[i]) + return false; + return true; + } +}; + +/// The plan: which files to rewrite, which manifests to keep. +struct BinPackPlan +{ + /// Bins of small files to merge. + std::vector bins; + /// Manifest paths to carry forward unchanged (delete manifests + data manifests with no small files). + std::unordered_set carry_forward_manifest_paths; + /// Total statistics across removed files. + Int64 removed_data_files = 0; + Int64 removed_records = 0; + Int64 removed_files_size = 0; + /// Number of distinct partitions affected. + Int64 num_partitions = 0; + /// The current snapshot id to use as parent. + Int64 current_snapshot_id = -1; + /// The partition spec to use for new manifests. + Poco::JSON::Object::Ptr partition_spec; + Int64 partition_spec_id = 0; + std::vector partition_columns; + DataTypes partition_types; +}; + + +BinPackPlan buildBinPackPlan( + Poco::JSON::Object::Ptr metadata_object, + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + ContextPtr context, + UInt64 min_file_size, + UInt64 target_file_size) +{ + LoggerPtr log = getLogger("IcebergBinPack::buildPlan"); + BinPackPlan plan; + + if (!metadata_object->has(f_current_snapshot_id)) + return plan; + Int64 current_snapshot_id = metadata_object->getValue(f_current_snapshot_id); + if (current_snapshot_id < 0) + return plan; + plan.current_snapshot_id = current_snapshot_id; + + String current_manifest_list_path; + auto snapshots = metadata_object->get(f_snapshots).extract(); + for (size_t i = 0; i < snapshots->size(); ++i) + { + const auto snapshot = snapshots->getObject(static_cast(i)); + if (snapshot->getValue(f_metadata_snapshot_id) == current_snapshot_id) + { + current_manifest_list_path = snapshot->getValue(f_manifest_list); + break; + } + } + if (current_manifest_list_path.empty()) + return plan; + + auto current_schema_id = metadata_object->getValue(f_current_schema_id); + + /// Resolve partition spec. + auto partition_spec_id = metadata_object->getValue(f_default_spec_id); + auto partitions_specs = metadata_object->getArray(f_partition_specs); + Poco::JSON::Object::Ptr partition_spec; + for (size_t i = 0; i < partitions_specs->size(); ++i) + { + auto candidate = partitions_specs->getObject(static_cast(i)); + if (candidate->getValue(f_spec_id) == partition_spec_id) + { + partition_spec = candidate; + break; + } + } + if (!partition_spec) + throw Exception( + ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, + "Iceberg metadata does not contain partition spec matching default-spec-id {}", + partition_spec_id); + + plan.partition_spec = partition_spec; + plan.partition_spec_id = partition_spec_id; + + auto spec_fields = partition_spec->getArray(f_fields); + std::vector partition_columns; + for (UInt32 i = 0; i < spec_fields->size(); ++i) + partition_columns.push_back(spec_fields->getObject(i)->getValue(f_name)); + plan.partition_columns = partition_columns; + + /// Resolve partition types. + auto schemas = metadata_object->getArray(f_schemas); + Poco::JSON::Object::Ptr current_schema; + for (size_t i = 0; i < schemas->size(); ++i) + { + if (schemas->getObject(static_cast(i))->getValue(f_schema_id) == current_schema_id) + { + current_schema = schemas->getObject(static_cast(i)); + break; + } + } + if (!current_schema) + throw Exception( + ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, + "Iceberg metadata does not contain schema matching current-schema-id {}", + current_schema_id); + + /// Build partition types from the partitioner. + for (UInt32 i = 0; i < schemas->size(); ++i) + persistent_table_components.schema_processor->addIcebergTableSchema(schemas->getObject(i), context); + + auto fields_characteristics = persistent_table_components.schema_processor->tryGetFieldsCharacteristics( + static_cast(current_schema_id), {}); + Block spec_sample_block; + for (const auto & nt : fields_characteristics) + spec_sample_block.insert(ColumnWithTypeAndName(nt.type, nt.name)); + auto shared_sample = std::make_shared(std::move(spec_sample_block)); + if (!partition_columns.empty()) + plan.partition_types = ChunkPartitioner(spec_fields, current_schema->getArray(f_fields), context, shared_sample).getResultTypes(); + + /// Scan the current manifest list. + auto manifest_list = getManifestList( + object_storage, persistent_table_components, context, + IcebergPathFromMetadata::deserialize(current_manifest_list_path), + log, secondary_storages); + + /// Collect files per partition. + using PartitionFiles = std::vector; + std::unordered_map partition_files; + /// Track manifest paths with no small files to carry forward. + std::unordered_set manifests_with_only_large_files; + + for (const auto & manifest_file : manifest_list) + { + if (manifest_file.content_type == ManifestFileContentType::DELETE) + { + plan.carry_forward_manifest_paths.insert(manifest_file.manifest_file_path.serialize()); + continue; + } + + auto files_handle = getManifestFileEntriesHandle( + object_storage, persistent_table_components, context, log, + manifest_file, static_cast(current_schema_id), secondary_storages); + + bool has_small_files = false; + for (const auto & data_file : files_handle.getFilesWithoutDeleted(FileContentType::DATA)) + { + const auto & entry = data_file->parsed_entry; + if (static_cast(entry->file_size_in_bytes) < min_file_size) + { + has_small_files = true; + SmallFileEntry small_entry; + small_entry.file_path = entry->file_path_key; + small_entry.record_count = entry->record_count; + small_entry.file_size_in_bytes = entry->file_size_in_bytes; + small_entry.file_format = entry->file_format; + small_entry.partition_key = entry->partition_key_value; + small_entry.sort_order_id = entry->sort_order_id; + small_entry.snapshot_id = entry->parsed_snapshot_id; + if (!small_entry.snapshot_id.has_value()) + small_entry.snapshot_id = manifest_file.added_snapshot_id; + small_entry.sequence_number = entry->parsed_sequence_number; + if (!small_entry.sequence_number.has_value()) + small_entry.sequence_number = manifest_file.added_sequence_number; + small_entry.file_sequence_number = entry->parsed_file_sequence_number; + if (!small_entry.file_sequence_number.has_value()) + small_entry.file_sequence_number = manifest_file.added_sequence_number; + + /// Carry over per-column stats. + for (const auto & [field_id, col_info] : entry->columns_infos) + { + if (col_info.bytes_size.has_value()) + small_entry.column_stats.column_sizes.emplace_back(field_id, *col_info.bytes_size); + if (col_info.rows_count.has_value()) + small_entry.column_stats.value_counts.emplace_back(field_id, *col_info.rows_count); + if (col_info.nulls_count.has_value()) + small_entry.column_stats.null_value_counts.emplace_back(field_id, *col_info.nulls_count); + } + for (const auto & [field_id, bounds] : entry->value_bounds) + { + if (!bounds.first.isNull()) + small_entry.column_stats.lower_bounds.emplace_back(field_id, bounds.first.safeGet()); + if (!bounds.second.isNull()) + small_entry.column_stats.upper_bounds.emplace_back(field_id, bounds.second.safeGet()); + } + + partition_files[entry->partition_key_value].push_back(std::move(small_entry)); + } + } + + if (!has_small_files) + manifests_with_only_large_files.insert(manifest_file.manifest_file_path.serialize()); + } + + /// Carry forward manifests that only have large files. + for (const auto & path : manifests_with_only_large_files) + plan.carry_forward_manifest_paths.insert(path); + + if (partition_files.empty()) + { + LOG_INFO(log, "No small files found below threshold {} bytes; nothing to compact", min_file_size); + return plan; + } + + /// Group small files into bins per partition. + for (auto & [partition_key, files] : partition_files) + { + /// Need at least 2 files to make compaction worthwhile. + if (files.size() < 2) + { + /// Not enough files to merge — the manifest containing these is NOT carried forward as-is + /// because it also holds the small files. The existing compaction logic will write a + /// data manifest for the untouched partition in the new manifest list below. + continue; + } + + Bin current_bin; + current_bin.partition_key = partition_key; + + for (auto & entry : files) + { + if (current_bin.total_bytes + entry.file_size_in_bytes > static_cast(target_file_size) + && !current_bin.files.empty()) + { + plan.bins.push_back(std::move(current_bin)); + current_bin = Bin{}; + current_bin.partition_key = partition_key; + } + + current_bin.total_bytes += entry.file_size_in_bytes; + current_bin.total_records += entry.record_count; + plan.removed_data_files++; + plan.removed_records += entry.record_count; + plan.removed_files_size += entry.file_size_in_bytes; + current_bin.files.push_back(std::move(entry)); + } + + if (current_bin.files.size() >= 2) + plan.bins.push_back(std::move(current_bin)); + else + { + /// Undo the stats for a single-file bin (not worth merging). + for (const auto & f : current_bin.files) + { + plan.removed_data_files--; + plan.removed_records -= f.record_count; + plan.removed_files_size -= f.file_size_in_bytes; + } + } + } + + plan.num_partitions = 0; + { + std::unordered_set affected_partitions; + for (const auto & bin : plan.bins) + affected_partitions.insert(bin.partition_key); + plan.num_partitions = static_cast(affected_partitions.size()); + } + + LOG_INFO(log, "Bin-pack plan: {} bins across {} partitions, {} small files totalling {} bytes", + plan.bins.size(), plan.num_partitions, plan.removed_data_files, plan.removed_files_size); + + return plan; +} + +} // anonymous namespace + + +bool executeBinPackCompaction( + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + const DataLakeStorageSettings & data_lake_settings, + SharedHeader sample_block, + ContextPtr context, + const String & write_format, + std::shared_ptr catalog, + const StorageID & table_id) +{ + LoggerPtr log = getLogger("IcebergBinPack"); + + const auto & settings = context->getSettingsRef(); + UInt64 target_size = settings[Setting::iceberg_target_data_file_size_bytes]; + UInt64 min_size = settings[Setting::iceberg_min_data_file_size_bytes]; + + const auto [metadata_version, metadata_file_path, _] = getLatestOrExplicitMetadataFileAndVersion( + object_storage, + persistent_table_components.table_path, + data_lake_settings, + persistent_table_components.metadata_cache, + context, + log.get(), + persistent_table_components.table_uuid, + persistent_table_components.metadata_compression_method, + /* force_fetch_latest_metadata */ true, + /* ignore_explicit_metadata_file_path */ true); + + auto metadata_object = getMetadataJSONObject( + metadata_file_path, + object_storage, + persistent_table_components.metadata_cache, + context, + log, + persistent_table_components.metadata_compression_method, + persistent_table_components.table_uuid); + + const Int32 format_version = metadata_object->getValue(f_format_version); + if (format_version < 2) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bin-packing compaction is supported only for Iceberg format_version >= 2"); + + /// Build the plan. + auto plan = buildBinPackPlan( + metadata_object, persistent_table_components, object_storage, + secondary_storages, context, min_size, target_size); + + if (plan.bins.empty()) + { + LOG_INFO(log, "No bins to compact; table is already optimally packed"); + return true; + } + + const auto & path_resolver = persistent_table_components.path_resolver; + CompressionMethod compression_method = persistent_table_components.metadata_compression_method; + + FileNamesGenerator generator( + path_resolver.getTableLocation(), false, compression_method, write_format); + generator.setVersion(metadata_version + 1); + + MetadataGenerator metadata_generator(metadata_object); + + /// Track new files for cleanup on failure. + std::vector new_data_file_paths; + std::vector new_manifest_paths; + IcebergPathFromMetadata manifest_list_path; + + auto cleanup = [&]() + { + for (const auto & p : new_data_file_paths) + { + try { object_storage->removeObjectIfExists(StoredObject(path_resolver.resolve(p))); } + catch (...) { tryLogCurrentException(log, "Cleanup: failed to remove data file"); } + } + for (const auto & p : new_manifest_paths) + { + try { object_storage->removeObjectIfExists(StoredObject(path_resolver.resolve(p))); } + catch (...) { tryLogCurrentException(log, "Cleanup: failed to remove manifest file"); } + } + if (!manifest_list_path.empty()) + { + try { object_storage->removeObjectIfExists(StoredObject(path_resolver.resolve(manifest_list_path))); } + catch (...) { tryLogCurrentException(log, "Cleanup: failed to remove manifest list"); } + } + }; + + try + { + /// Resolve schema for the column mapper. + auto current_schema_id = metadata_object->getValue(f_current_schema_id); + auto schemas = metadata_object->getArray(f_schemas); + Poco::JSON::Object::Ptr current_schema; + for (size_t i = 0; i < schemas->size(); ++i) + { + if (schemas->getObject(static_cast(i))->getValue(f_schema_id) == current_schema_id) + { + current_schema = schemas->getObject(static_cast(i)); + break; + } + } + if (!current_schema) + throw Exception(ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, + "Missing schema for current-schema-id {}", current_schema_id); + + /// Phase 1: Read small files and write merged data files. + /// For each bin, read all source files and write a merged file via MultipleFileWriter. + Int64 total_added_files = 0; + Int64 total_added_records = 0; + Int64 total_added_files_size = 0; + + /// Track info per bin for manifest writing. + struct BinResult + { + Row partition_key; + std::vector merged_file_paths; + std::vector merged_file_row_counts; + std::vector merged_file_byte_counts; + /// The old files that were replaced. + std::vector old_file_paths; + std::vector old_file_row_counts; + std::vector old_file_byte_counts; + std::vector old_file_lineage; + std::vector old_file_stats; + std::vector old_file_formats; + std::vector> old_file_sort_order_ids; + }; + std::vector bin_results; + + for (auto & bin : plan.bins) + { + BinResult result; + result.partition_key = bin.partition_key; + + /// Prepare the MultipleFileWriter for this bin. + MultipleFileWriter writer( + /* max_data_file_num_rows */ 0, /// no row limit; size limit is used + /* max_data_file_num_bytes */ target_size, + current_schema->getArray(f_fields), + generator, + path_resolver, + object_storage, + context, + std::nullopt, /// format_settings + write_format, + sample_block, + [&](const std::string & path) + { + new_data_file_paths.push_back(IcebergPathFromMetadata::deserialize(path)); + }); + + /// Read each source file and feed into the writer. + for (auto & file_entry : bin.files) + { + auto [resolved_storage, resolved_key] = resolveObjectStorageForPath( + persistent_table_components.table_location, + file_entry.file_path.serialize(), + object_storage, secondary_storages, context, + path_resolver); + + RelativePathWithMetadata object_info(resolved_key); + ObjectStoragePtr storage_to_use = resolved_storage ? resolved_storage : object_storage; + auto read_buffer = createReadBuffer(object_info, storage_to_use, context, log); + + auto parser_shared_resources = std::make_shared( + settings, /*num_streams_=*/1); + + auto input_format = FormatFactory::instance().getInput( + file_entry.file_format.empty() ? write_format : file_entry.file_format, + *read_buffer, + *sample_block, + context, + 8192, + std::nullopt, /// format_settings + parser_shared_resources, + std::make_shared(nullptr, context, nullptr, nullptr, nullptr), + true, /// is_remote_fs + CompressionMethod::None, + false); + + while (true) + { + auto chunk = input_format->read(); + if (chunk.empty()) + break; + writer.consume(chunk); + } + + /// Track old file info for the delete manifest. + result.old_file_paths.push_back(file_entry.file_path); + result.old_file_row_counts.push_back(static_cast(file_entry.record_count)); + result.old_file_byte_counts.push_back(static_cast(file_entry.file_size_in_bytes)); + result.old_file_formats.push_back(file_entry.file_format); + result.old_file_sort_order_ids.push_back(file_entry.sort_order_id); + result.old_file_stats.push_back(std::move(file_entry.column_stats)); + + DataFileEntryLineage lineage; + lineage.added_snapshot_id = file_entry.snapshot_id; + lineage.sequence_number = file_entry.sequence_number; + lineage.file_sequence_number = file_entry.file_sequence_number; + lineage.status_override = ManifestEntryStatus::DELETED; + result.old_file_lineage.push_back(lineage); + } + + writer.finalize(); + + result.merged_file_paths = writer.getDataFiles(); + result.merged_file_row_counts = writer.getDataFileRowCounts(); + result.merged_file_byte_counts = writer.getDataFileByteCounts(); + + for (size_t i = 0; i < result.merged_file_paths.size(); ++i) + { + total_added_files++; + total_added_records += static_cast(result.merged_file_row_counts[i]); + total_added_files_size += static_cast(result.merged_file_byte_counts[i]); + } + + bin_results.push_back(std::move(result)); + } + + /// Phase 2: Generate the replace snapshot. + auto generated_metadata_info = generator.generateMetadataPathWithInfo(); + auto snapshot_result = metadata_generator.generateReplaceSnapshot( + generator, + generated_metadata_info.path, + plan.current_snapshot_id, + total_added_files, + total_added_records, + total_added_files_size, + plan.removed_data_files, + plan.removed_records, + plan.removed_files_size, + plan.num_partitions); + + /// Phase 3: Write manifest files. + /// We write two types of manifests: + /// - Delete manifests: DELETED entries for old files (one per bin) + /// - Add manifests: ADDED entries for new merged files (one per bin) + std::vector all_new_manifest_paths; + std::vector all_manifest_sizes; + std::vector all_existing_counts; + std::vector all_entry_partition_spec_ids; + + for (auto & bin_result : bin_results) + { + /// Delete manifest: old files marked DELETED. + { + auto manifest_path = generator.generateManifestEntryName(); + auto storage_path = path_resolver.resolve(manifest_path); + new_manifest_paths.push_back(manifest_path); + + auto buf = object_storage->writeObject( + StoredObject(storage_path), WriteMode::Rewrite, std::nullopt, + DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); + + generateManifestFile( + metadata_object, + plan.partition_columns, + bin_result.partition_key, + plan.partition_types, + bin_result.old_file_paths, + bin_result.old_file_row_counts, + bin_result.old_file_byte_counts, + std::nullopt, /// data_file_statistics + sample_block, + snapshot_result.snapshot, + write_format, + plan.partition_spec, + plan.partition_spec_id, + *buf, + FileContentType::DATA, + std::nullopt, /// user_defined_sequence_number + {}, /// per_file_stats + bin_result.old_file_formats, + bin_result.old_file_stats, + bin_result.old_file_sort_order_ids, + bin_result.old_file_lineage); + + buf->finalize(); + Int64 manifest_size = buf->count(); + if (manifest_size == 0) + manifest_size = object_storage->getObjectMetadata(storage_path, false).size_bytes; + + all_new_manifest_paths.push_back(manifest_path); + all_manifest_sizes.push_back(manifest_size); + /// The DELETED manifest has existing (really: deleted) file counts for manifest-list accounting. + Int64 min_seq = std::numeric_limits::max(); + for (const auto & lineage : bin_result.old_file_lineage) + min_seq = std::min(min_seq, lineage.sequence_number.value_or(0)); + all_existing_counts.push_back( + {static_cast(bin_result.old_file_paths.size()), + static_cast(std::accumulate(bin_result.old_file_row_counts.begin(), bin_result.old_file_row_counts.end(), 0UL)), + min_seq}); + all_entry_partition_spec_ids.push_back(plan.partition_spec_id); + } + + /// Add manifest: new merged files. + { + auto manifest_path = generator.generateManifestEntryName(); + auto storage_path = path_resolver.resolve(manifest_path); + new_manifest_paths.push_back(manifest_path); + + auto buf = object_storage->writeObject( + StoredObject(storage_path), WriteMode::Rewrite, std::nullopt, + DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); + + generateManifestFile( + metadata_object, + plan.partition_columns, + bin_result.partition_key, + plan.partition_types, + bin_result.merged_file_paths, + bin_result.merged_file_row_counts, + bin_result.merged_file_byte_counts, + std::nullopt, /// data_file_statistics + sample_block, + snapshot_result.snapshot, + write_format, + plan.partition_spec, + plan.partition_spec_id, + *buf, + FileContentType::DATA); + + buf->finalize(); + Int64 manifest_size = buf->count(); + if (manifest_size == 0) + manifest_size = object_storage->getObjectMetadata(storage_path, false).size_bytes; + + all_new_manifest_paths.push_back(manifest_path); + all_manifest_sizes.push_back(manifest_size); + all_existing_counts.push_back({0, 0, 0}); /// New files: no existing counts. + all_entry_partition_spec_ids.push_back(plan.partition_spec_id); + } + } + + /// Phase 4: Write manifest list. + { + auto storage_manifest_list_path = path_resolver.resolve(snapshot_result.manifest_list_path); + manifest_list_path = snapshot_result.manifest_list_path; + + auto buf = object_storage->writeObject( + StoredObject(storage_manifest_list_path), WriteMode::Rewrite, std::nullopt, + DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); + + generateManifestList( + path_resolver, + metadata_object, + object_storage, + secondary_storages, + context, + all_new_manifest_paths, + snapshot_result.snapshot, + all_manifest_sizes, + *buf, + FileContentType::DATA, + false, /// use_previous_snapshots + {}, /// per_entry_content_types + all_existing_counts, + plan.carry_forward_manifest_paths, + all_entry_partition_spec_ids); + + buf->finalize(); + } + + /// Phase 5: Commit metadata. + { + std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + Poco::JSON::Stringifier::stringify(metadata_object, oss, 4); + std::string json_representation = removeEscapedSlashes(oss.str()); + + auto hint_path = generator.generateVersionHint(); + + const bool catalog_writes_metadata_file = catalog && catalog->isTransactional(); + if (!catalog_writes_metadata_file + && !writeMetadataFileAndVersionHint( + path_resolver, + generated_metadata_info, + json_representation, + hint_path, + object_storage, + context, + data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) + { + LOG_INFO(log, "Bin-pack commit conflict detected, cleaning up"); + cleanup(); + return false; + } + + if (catalog) + { + auto catalog_filename = path_resolver.resolveForCatalog(generated_metadata_info.path); + const auto & [namespace_name, table_name] = DataLake::parseTableName(table_id.getTableName()); + if (!catalog->updateMetadata(namespace_name, table_name, catalog_filename, snapshot_result.snapshot)) + { + LOG_INFO(log, "Bin-pack commit conflict via catalog, cleaning up"); + cleanup(); + return false; + } + } + } + + LOG_INFO(log, "Bin-pack compaction committed: {} bins, {} new files ({} records, {} bytes), " + "{} old files removed ({} records, {} bytes)", + plan.bins.size(), total_added_files, total_added_records, total_added_files_size, + plan.removed_data_files, plan.removed_records, plan.removed_files_size); + return true; + } + catch (...) + { + cleanup(); + throw; + } +} + +} + +#endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h new file mode 100644 index 000000000000..1abc28e5ff7e --- /dev/null +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h @@ -0,0 +1,42 @@ +#pragma once + +#include "config.h" + +#if USE_AVRO + +#include +#include +#include +#include +#include + +namespace DataLake +{ +class ICatalog; +} + +namespace DB::Iceberg +{ + +/// Execute bin-packing compaction for an Iceberg table: merge small data files into +/// larger ones, producing a `replace` snapshot that atomically swaps the old files +/// for the merged results. Only data files smaller than `iceberg_min_data_file_size_bytes` +/// are candidates; each bin targets `iceberg_target_data_file_size_bytes`. +/// +/// Leaves all other files, manifests, and snapshot history untouched. +/// +/// Returns true on successful commit, false on commit conflict (caller should retry). +bool executeBinPackCompaction( + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + const DataLakeStorageSettings & data_lake_settings, + SharedHeader sample_block, + ContextPtr context, + const String & write_format, + std::shared_ptr catalog, + const StorageID & table_id); + +} + +#endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 7f7d7211c680..adf54cfb13c5 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -68,6 +68,7 @@ #include #include #include +#include #include #include #include @@ -512,27 +513,42 @@ IcebergMetadata::getIcebergDataSnapshot(Poco::JSON::Object::Ptr metadata_object, bool IcebergMetadata::optimize( const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & format_settings) { - if (context->getSettingsRef()[Setting::allow_experimental_iceberg_compaction]) - { - const auto sample_block = std::make_shared(metadata_snapshot->getSampleBlock()); - auto snapshots_info = getHistory(context); - compactIcebergTable( - snapshots_info, - persistent_components, - object_storage, - secondary_storages, - data_lake_settings, - format_settings, - sample_block, - context, - write_format); - return true; - } - else - { + if (!context->getSettingsRef()[Setting::allow_experimental_iceberg_compaction]) throw Exception( ErrorCodes::BAD_ARGUMENTS, "Enable 'allow_experimental_iceberg_compaction' setting to call optimize for iceberg tables."); + + static constexpr size_t MAX_BIN_PACK_RETRIES = 100; + + const auto sample_block = std::make_shared(metadata_snapshot->getSampleBlock()); + + for (size_t attempt = 0; attempt < MAX_BIN_PACK_RETRIES; ++attempt) + { + if (attempt > 0) + LOG_INFO(log, "Retrying bin-pack compaction (attempt {}/{})", attempt + 1, MAX_BIN_PACK_RETRIES); + + if (Iceberg::executeBinPackCompaction( + persistent_components, + object_storage, + *secondary_storages, + data_lake_settings, + sample_block, + context, + write_format, + /* catalog */ nullptr, + /* table_id */ StorageID::createEmpty())) + { + if (persistent_components.metadata_cache) + { + persistent_components.metadata_cache->remove(persistent_components.table_path); + if (persistent_components.table_uuid) + persistent_components.metadata_cache->remove(*persistent_components.table_uuid); + } + return true; + } } + + throw Exception(ErrorCodes::LOGICAL_ERROR, + "Bin-pack compaction failed to commit after {} attempts", MAX_BIN_PACK_RETRIES); } bool IcebergMetadata::optimizeManifestFiles( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp index 0adbf4f2c0e5..9c4fdff05a53 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp @@ -723,9 +723,14 @@ void generateManifestFile( const DataFileEntryLineage * entry_lineage = per_file_entry_lineage.empty() ? nullptr : &per_file_entry_lineage[file_idx]; - manifest.field(Iceberg::f_status) - = avro::GenericDatum(entry_lineage ? static_cast(ManifestEntryStatus::EXISTING) - : static_cast(ManifestEntryStatus::ADDED)); + ManifestEntryStatus entry_status; + if (entry_lineage && entry_lineage->status_override) + entry_status = *entry_lineage->status_override; + else if (entry_lineage) + entry_status = ManifestEntryStatus::EXISTING; + else + entry_status = ManifestEntryStatus::ADDED; + manifest.field(Iceberg::f_status) = avro::GenericDatum(static_cast(entry_status)); Int64 snapshot_id = (entry_lineage && entry_lineage->added_snapshot_id) ? *entry_lineage->added_snapshot_id : new_snapshot->getValue(Iceberg::f_metadata_snapshot_id); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h index a7c6c6a322c5..7c2004c11563 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h @@ -61,11 +61,14 @@ struct DataFileColumnStatistics }; /// Per-file manifest-entry lineage (`added_snapshot_id`, data `sequence_number` and `file_sequence_number`) carried over for a manifest-only rewrite. +/// When `status_override` is set, the entry is written with that status instead of the default +/// EXISTING/ADDED logic. Used by bin-packing compaction to produce DELETED entries. struct DataFileEntryLineage { std::optional added_snapshot_id; std::optional sequence_number; std::optional file_sequence_number; + std::optional status_override; }; /// Read a data-file sidecar and return its contents in Iceberg wire format. diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 7c232a26b8fe..327fe8347fc6 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -640,6 +640,121 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateManifestOnlySna return {new_snapshot, manifest_list_path}; } +MetadataGenerator::NextMetadataResult MetadataGenerator::generateReplaceSnapshot( + FileNamesGenerator & generator, + const Iceberg::IcebergPathFromMetadata & metadata_file_path, + Int64 parent_snapshot_id, + Int64 added_data_files, + Int64 added_records, + Int64 added_files_size, + Int64 removed_data_files, + Int64 removed_records, + Int64 removed_files_size, + Int64 num_partitions) +{ + int format_version = metadata_object->getValue(Iceberg::f_format_version); + + for (const auto * field : {Iceberg::f_metadata_log, Iceberg::f_snapshot_log}) + if (!metadata_object->has(field)) + metadata_object->set(field, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + + if (!metadata_object->has(Iceberg::f_snapshots)) + throw Exception( + ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, + "Metadata has a current snapshot with id {} but no `snapshots` list", + parent_snapshot_id); + + Poco::JSON::Object::Ptr new_snapshot = new Poco::JSON::Object; + if (format_version > 1) + { + auto sequence_number = getMaxSequenceNumber() + 1; + new_snapshot->set(Iceberg::f_metadata_sequence_number, sequence_number); + metadata_object->set(Iceberg::f_last_sequence_number, sequence_number); + } + Int64 snapshot_id = static_cast(dis(gen)); + + auto manifest_list_path = generator.generateManifestListName(snapshot_id, format_version); + new_snapshot->set(Iceberg::f_metadata_snapshot_id, snapshot_id); + new_snapshot->set(Iceberg::f_parent_snapshot_id, parent_snapshot_id); + + auto now = std::chrono::system_clock::now(); + auto ms = duration_cast(now.time_since_epoch()); + Int64 timestamp = ms.count(); + new_snapshot->set(Iceberg::f_timestamp_ms, timestamp); + metadata_object->set(Iceberg::f_last_updated_ms, timestamp); + + auto parent_snapshot = getParentSnapshot(parent_snapshot_id); + + Poco::JSON::Object::Ptr summary = new Poco::JSON::Object; + summary->set(Iceberg::f_operation, Iceberg::f_replace); + summary->set(Iceberg::f_added_data_files, std::to_string(added_data_files)); + summary->set(Iceberg::f_added_records, std::to_string(added_records)); + summary->set(Iceberg::f_added_files_size, std::to_string(added_files_size)); + summary->set(Iceberg::f_deleted_data_files, std::to_string(removed_data_files)); + summary->set(Iceberg::f_removed_data_files, std::to_string(removed_data_files)); + summary->set(Iceberg::f_deleted_records, std::to_string(removed_records)); + summary->set(Iceberg::f_removed_files_size, std::to_string(removed_files_size)); + summary->set(Iceberg::f_changed_partition_count, std::to_string(num_partitions)); + + /// Compute total-* counters: parent totals + added - removed. + setSnapshotTotals( + summary, + parent_snapshot, + /*added_records=*/added_records - removed_records, + /*added_files_size=*/added_files_size - removed_files_size, + /*added_data_files=*/added_data_files - removed_data_files, + /*added_delete_files=*/0, + /*added_position_deletes=*/0, + /*added_equality_deletes=*/0); + new_snapshot->set(Iceberg::f_summary, summary); + + new_snapshot->set(Iceberg::f_schema_id, metadata_object->getValue(Iceberg::f_current_schema_id)); + new_snapshot->set(Iceberg::f_manifest_list, manifest_list_path.serialize()); + + if (format_version >= 3) + { + Int64 next_row_id = metadata_object->has(Iceberg::f_next_row_id) && !metadata_object->isNull(Iceberg::f_next_row_id) + ? metadata_object->getValue(Iceberg::f_next_row_id) + : 0; + new_snapshot->set(Iceberg::f_first_row_id, next_row_id); + new_snapshot->set(Iceberg::f_added_rows, added_records); + metadata_object->set(Iceberg::f_next_row_id, next_row_id + added_records); + } + + getOrCreateArray(metadata_object, Iceberg::f_snapshots)->add(new_snapshot); + metadata_object->set(Iceberg::f_current_snapshot_id, snapshot_id); + + if (!metadata_object->has(Iceberg::f_refs)) + metadata_object->set(Iceberg::f_refs, Poco::JSON::Object::Ptr(new Poco::JSON::Object)); + + if (!metadata_object->getObject(Iceberg::f_refs)->has(Iceberg::f_main)) + { + Poco::JSON::Object::Ptr branch = new Poco::JSON::Object; + branch->set(Iceberg::f_metadata_snapshot_id, snapshot_id); + branch->set(Iceberg::f_type, Iceberg::f_branch); + metadata_object->getObject(Iceberg::f_refs)->set(Iceberg::f_main, branch); + } + else + { + metadata_object->getObject(Iceberg::f_refs)->getObject(Iceberg::f_main)->set(Iceberg::f_metadata_snapshot_id, snapshot_id); + } + + { + Poco::JSON::Object::Ptr new_metadata_item = new Poco::JSON::Object; + new_metadata_item->set(Iceberg::f_metadata_file, metadata_file_path.serialize()); + new_metadata_item->set(Iceberg::f_timestamp_ms, timestamp); + getOrCreateArray(metadata_object, Iceberg::f_metadata_log)->add(new_metadata_item); + } + { + Poco::JSON::Object::Ptr new_snapshot_item = new Poco::JSON::Object; + new_snapshot_item->set(Iceberg::f_metadata_snapshot_id, snapshot_id); + new_snapshot_item->set(Iceberg::f_timestamp_ms, timestamp); + getOrCreateArray(metadata_object, Iceberg::f_snapshot_log)->add(new_snapshot_item); + } + + return {new_snapshot, manifest_list_path}; +} + void MetadataGenerator::generateDropColumnMetadata(const String & column_name) { const auto next_schema_id = getNextSchemaId(metadata_object); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index 33ff405a65d7..ccb145a26858 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -47,6 +47,19 @@ class MetadataGenerator const Iceberg::IcebergPathFromMetadata & metadata_file_path, Int64 parent_snapshot_id); + /// Create a `replace` snapshot for bin-packing compaction: atomically removes small files and adds merged files. + NextMetadataResult generateReplaceSnapshot( + FileNamesGenerator & generator, + const Iceberg::IcebergPathFromMetadata & metadata_file_path, + Int64 parent_snapshot_id, + Int64 added_data_files, + Int64 added_records, + Int64 added_files_size, + Int64 removed_data_files, + Int64 removed_records, + Int64 removed_files_size, + Int64 num_partitions); + void generateAddColumnMetadata(const String & column_name, DataTypePtr type); void generateDropColumnMetadata(const String & column_name); /// Returns false when the column already has the requested type (no metadata change). diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp new file mode 100644 index 000000000000..484ab231740a --- /dev/null +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp @@ -0,0 +1,225 @@ +#include "config.h" + +#if USE_AVRO + +#include + +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; +using namespace DB::Iceberg; + +namespace +{ + +/// Build minimal metadata suitable for snapshot generation. +Poco::JSON::Object::Ptr makeMetadataForReplace() +{ + auto metadata = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + metadata->set(f_format_version, 2); + metadata->set(f_current_schema_id, 0); + metadata->set(f_last_column_id, 1); + metadata->set(f_default_spec_id, 0); + metadata->set(f_last_sequence_number, Int64(2)); + metadata->set(f_table_uuid, "test-uuid-1234"); + + auto schemas = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto schema = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + schema->set(f_schema_id, 0); + schema->set(f_type, "struct"); + auto fields = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto field = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + field->set(f_id, 1); + field->set(f_name, "x"); + field->set(f_required, true); + field->set(f_type, "int"); + fields->add(field); + schema->set(f_fields, fields); + schemas->add(schema); + metadata->set(f_schemas, schemas); + + /// Partition specs. + auto specs = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto spec = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + spec->set(f_spec_id, 0); + spec->set(f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + specs->add(spec); + metadata->set(f_partition_specs, specs); + + /// Create a parent snapshot with known totals. + auto snapshots = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto parent_snapshot = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + parent_snapshot->set(f_metadata_snapshot_id, Int64(100)); + parent_snapshot->set(f_timestamp_ms, Int64(1000)); + parent_snapshot->set(f_metadata_sequence_number, Int64(1)); + parent_snapshot->set(f_manifest_list, "s3://bucket/metadata/snap-100-0.avro"); + + auto parent_summary = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + parent_summary->set(f_operation, f_append); + parent_summary->set(f_total_records, "1000"); + parent_summary->set(f_total_files_size, "50000"); + parent_summary->set(f_total_data_files, "10"); + parent_summary->set(f_total_delete_files, "0"); + parent_summary->set(f_total_position_deletes, "0"); + parent_summary->set(f_total_equality_deletes, "0"); + parent_snapshot->set(f_summary, parent_summary); + + snapshots->add(parent_snapshot); + metadata->set(f_snapshots, snapshots); + metadata->set(f_current_snapshot_id, Int64(100)); + + return metadata; +} + +} + + +TEST(IcebergBinPackRewrite, ReplaceSnapshotSummaryCounters) +{ + auto metadata = makeMetadataForReplace(); + MetadataGenerator gen(metadata); + + FileNamesGenerator file_gen("s3://bucket/table/", false, CompressionMethod::None, "Parquet"); + file_gen.setVersion(2); + + auto metadata_path = file_gen.generateMetadataPathWithInfo(); + + auto result = gen.generateReplaceSnapshot( + file_gen, + metadata_path.path, + /*parent_snapshot_id=*/100, + /*added_data_files=*/2, + /*added_records=*/1000, + /*added_files_size=*/40000, + /*removed_data_files=*/8, + /*removed_records=*/800, + /*removed_files_size=*/35000, + /*num_partitions=*/3); + + ASSERT_NE(result.snapshot, nullptr); + + auto summary = result.snapshot->getObject(f_summary); + ASSERT_NE(summary, nullptr); + + /// Operation must be `replace`. + EXPECT_EQ(summary->getValue(f_operation), f_replace); + + /// Added counters. + EXPECT_EQ(summary->getValue(f_added_data_files), "2"); + EXPECT_EQ(summary->getValue(f_added_records), "1000"); + EXPECT_EQ(summary->getValue(f_added_files_size), "40000"); + + /// Removed counters. + EXPECT_EQ(summary->getValue(f_deleted_data_files), "8"); + EXPECT_EQ(summary->getValue(f_removed_data_files), "8"); + EXPECT_EQ(summary->getValue(f_deleted_records), "800"); + EXPECT_EQ(summary->getValue(f_removed_files_size), "35000"); + + /// Partition count. + EXPECT_EQ(summary->getValue(f_changed_partition_count), "3"); + + /// Total-* counters: parent + (added - removed). + /// total_records: 1000 + (1000 - 800) = 1200 + EXPECT_EQ(summary->getValue(f_total_records), "1200"); + /// total_files_size: 50000 + (40000 - 35000) = 55000 + EXPECT_EQ(summary->getValue(f_total_files_size), "55000"); + /// total_data_files: 10 + (2 - 8) = 4 + EXPECT_EQ(summary->getValue(f_total_data_files), "4"); +} + + +TEST(IcebergBinPackRewrite, ReplaceSnapshotSequenceNumberIncremented) +{ + auto metadata = makeMetadataForReplace(); + MetadataGenerator gen(metadata); + + FileNamesGenerator file_gen("s3://bucket/table/", false, CompressionMethod::None, "Parquet"); + file_gen.setVersion(2); + + auto metadata_path = file_gen.generateMetadataPathWithInfo(); + + auto result = gen.generateReplaceSnapshot( + file_gen, + metadata_path.path, + /*parent_snapshot_id=*/100, + /*added_data_files=*/1, + /*added_records=*/500, + /*added_files_size=*/20000, + /*removed_data_files=*/5, + /*removed_records=*/500, + /*removed_files_size=*/25000, + /*num_partitions=*/1); + + /// The new snapshot's sequence number must be > parent's (which was 2). + EXPECT_GT(result.snapshot->getValue(f_metadata_sequence_number), 2); + /// metadata.last-sequence-number must also advance. + EXPECT_GT(metadata->getValue(f_last_sequence_number), 2); +} + + +TEST(IcebergBinPackRewrite, DataFileEntryLineageStatusOverride) +{ + /// When status_override is set, the entry should use that status. + DataFileEntryLineage lineage; + lineage.added_snapshot_id = 42; + lineage.sequence_number = 1; + lineage.file_sequence_number = 1; + lineage.status_override = ManifestEntryStatus::DELETED; + + EXPECT_EQ(lineage.status_override.value(), ManifestEntryStatus::DELETED); +} + + +TEST(IcebergBinPackRewrite, DataFileEntryLineageNoOverrideDefaultsToExisting) +{ + /// When status_override is not set but lineage is present, the entry status + /// should be EXISTING (handled by generateManifestFile logic, but we test the struct). + DataFileEntryLineage lineage; + lineage.added_snapshot_id = 42; + lineage.sequence_number = 1; + lineage.file_sequence_number = 1; + + EXPECT_FALSE(lineage.status_override.has_value()); +} + + +TEST(IcebergBinPackRewrite, SnapshotSummaryReplaceOperationCounters) +{ + /// Build a SnapshotSummary with a replace update and verify totals. + SnapshotSummaryTotals parent_totals{ + .records = 1000, + .files_size = 50000, + .data_files = 10, + .delete_files = 0, + .position_deletes = 0, + .equality_deletes = 0}; + + SnapshotSummary summary( + SnapshotSummaryUpdateReplace{ + .added_files = 3, + .added_records = 800, + .added_files_size = 30000, + .deleted_data_files = 7, + .removed_records = 700, + .removed_files_size = 28000, + .num_partitions = 2}, + parent_totals); + + EXPECT_EQ(summary.getOperation(), SnapshotSummaryOperation::REPLACE); + + auto totals = summary.getTotals(); + /// total_records: 1000 + 800 - 700 = 1100 + EXPECT_EQ(totals.records, 1100); + /// total_files_size: 50000 + 30000 - 28000 = 52000 + EXPECT_EQ(totals.files_size, 52000); + /// total_data_files: 10 + 3 - 7 = 6 + EXPECT_EQ(totals.data_files, 6); +} + +#endif diff --git a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py new file mode 100644 index 000000000000..c5472a2b3caa --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py @@ -0,0 +1,181 @@ +"""Integration test for Iceberg bin-packing compaction via OPTIMIZE TABLE. + +Creates a table, inserts many small batches to produce many small data files, +runs OPTIMIZE TABLE, and verifies: +- fewer data files after compaction +- same row count and data integrity +- the snapshot has a `replace` operation +""" + +import json +import pytest + +from helpers.iceberg_utils import ( + create_iceberg_table, + get_uuid_str, + default_download_directory, +) + + +def _count_data_files(instance, table_name): + """Return the number of DATA files via system.iceberg_files.""" + result = instance.query( + f"SELECT count() FROM system.iceberg_files " + f"WHERE database = 'default' AND table = '{table_name}' AND content = 'DATA'" + ).strip() + return int(result) + + +def _get_latest_snapshot_summary(instance, table_name): + """Return the summary of the latest snapshot as a dict.""" + raw = instance.query( + f"SELECT toJSONString(summary) " + f"FROM system.iceberg_history " + f"WHERE database = 'default' AND table = '{table_name}' " + f"ORDER BY made_current_at DESC LIMIT 1" + ).strip() + return json.loads(raw) if raw else {} + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_rewrite(started_cluster_iceberg_no_spark, format_version): + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_rewrite_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + # Insert many small batches — each INSERT produces one data file. + num_batches = 10 + rows_per_batch = 100 + total_rows = num_batches * rows_per_batch + + for batch in range(num_batches): + values = ", ".join( + f"({batch * rows_per_batch + i}, 'row_{batch * rows_per_batch + i}')" + for i in range(rows_per_batch) + ) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + # Verify we have many data files. + files_before = _count_data_files(instance, table_name) + assert files_before == num_batches, ( + f"Expected {num_batches} data files before compaction, got {files_before}" + ) + + # Verify total row count. + count_before = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_before == total_rows + + # Capture data before compaction for integrity check. + data_before = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + + # Run OPTIMIZE TABLE with tiny thresholds so all files are candidates. + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, # 10 MB target + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, # all files < 10 MB are candidates + }, + ) + + # Drop and recreate the table to pick up the new metadata. + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + ) + + # Verify fewer data files. + files_after = _count_data_files(instance, table_name) + assert files_after < files_before, ( + f"Expected fewer files after compaction: before={files_before}, after={files_after}" + ) + + # Verify same row count. + count_after = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_after == total_rows, ( + f"Row count mismatch: before={total_rows}, after={count_after}" + ) + + # Verify data integrity. + data_after = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + assert data_after == data_before, "Data mismatch after compaction" + + # Verify the latest snapshot has a `replace` operation. + summary = _get_latest_snapshot_summary(instance, table_name) + assert summary.get("operation") == "replace", ( + f"Expected 'replace' operation in snapshot summary, got: {summary.get('operation')}" + ) + + # Verify summary counters. + assert int(summary.get("added-data-files", 0)) > 0 + assert int(summary.get("deleted-data-files", 0)) == num_batches + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_noop_when_no_small_files( + started_cluster_iceberg_no_spark, format_version +): + """When there are no small files, OPTIMIZE should be a no-op.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_noop_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64)", + format_version=format_version, + ) + + # Insert a single batch. + values = ", ".join(f"({i})" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + files_before = _count_data_files(instance, table_name) + assert files_before == 1 + + # OPTIMIZE with a very small threshold — but only 1 file, so nothing to merge. + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 1, + "iceberg_min_data_file_size_bytes": 1024 * 1024 * 1024, # 1 GB — everything is "small" + }, + ) + + # With only 1 file, there's nothing to bin-pack. File count stays. + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + ) + + files_after = _count_data_files(instance, table_name) + assert files_after == files_before, ( + f"Expected same number of files (nothing to compact): before={files_before}, after={files_after}" + ) From 23e7a529b070d8e452bc9eda508fbaafef9b7221 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Wed, 23 Sep 2026 21:24:34 +0200 Subject: [PATCH 02/11] Fix compilation error in IcebergMetadata --- .../ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index adf54cfb13c5..c9a0f61bbaa0 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -511,7 +511,7 @@ IcebergMetadata::getIcebergDataSnapshot(Poco::JSON::Object::Ptr metadata_object, } bool IcebergMetadata::optimize( - const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & format_settings) + const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & /*format_settings*/) { if (!context->getSettingsRef()[Setting::allow_experimental_iceberg_compaction]) throw Exception( From 43c15c4e0f55f59779d446b30ff8fecb002fd24a Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Wed, 23 Sep 2026 23:42:31 +0200 Subject: [PATCH 03/11] Explicitly specify SecondaryStorages in BinPackRewrite.h --- .../ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h index 1abc28e5ff7e..5722ee7849db 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h @@ -15,6 +15,11 @@ namespace DataLake class ICatalog; } +namespace DB +{ +struct SecondaryStorages; +} + namespace DB::Iceberg { From d10fcddf6dbe0c65faf65f927e7c30eb288f1be3 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 24 Sep 2026 17:27:48 +0200 Subject: [PATCH 04/11] Fix assert in gtest_bin_pack_rewrite --- .../DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp index 484ab231740a..d4dce4c20b95 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp @@ -102,10 +102,10 @@ TEST(IcebergBinPackRewrite, ReplaceSnapshotSummaryCounters) /*removed_files_size=*/35000, /*num_partitions=*/3); - ASSERT_NE(result.snapshot, nullptr); + ASSERT_NE(result.snapshot.get(), nullptr); auto summary = result.snapshot->getObject(f_summary); - ASSERT_NE(summary, nullptr); + ASSERT_NE(summary.get(), nullptr); /// Operation must be `replace`. EXPECT_EQ(summary->getValue(f_operation), f_replace); From 0357f1f8ebfb3b5da20903edcb71340bd1809936 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 24 Sep 2026 19:38:05 +0200 Subject: [PATCH 05/11] Link Avro library to unit_tests_dbms so headers can be included in test files --- src/CMakeLists.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 065d44432d74..e95950afe199 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -956,6 +956,10 @@ if (ENABLE_TESTS) target_link_libraries(unit_tests_dbms PRIVATE ch_contrib::parquet) endif() + if (TARGET ch_contrib::avrocpp) + target_link_libraries(unit_tests_dbms PRIVATE ch_contrib::avrocpp) + endif() + if (TARGET ch_contrib::silk_fibers) target_link_libraries(unit_tests_dbms PRIVATE ch_contrib::silk_fibers) endif() From 850d3e98411e6447b83a5a884e71ce6fee5c0ec3 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Mon, 28 Sep 2026 19:25:50 +0200 Subject: [PATCH 06/11] Address PR comments --- .../DataLakes/Iceberg/BinPackRewrite.cpp | 533 ++++++++++++------ .../DataLakes/Iceberg/MultipleFileWriter.cpp | 7 +- .../DataLakes/Iceberg/MultipleFileWriter.h | 1 + .../test_bin_pack_rewrite.py | 245 ++++++++ 4 files changed, 622 insertions(+), 164 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp index fe1ea5b4c797..bf4e44b46945 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -52,8 +52,8 @@ namespace DB::Iceberg namespace { -/// A single small data file selected for bin-packing. -struct SmallFileEntry +/// A single live data file recorded from a data manifest. +struct DataFileRecord { IcebergPathFromMetadata file_path; Int64 record_count; @@ -67,13 +67,34 @@ struct SmallFileEntry std::optional file_sequence_number; /// Per-column statistics from the source manifest. DataFileColumnStatistics column_stats; + /// The manifest this entry was read from. + String source_manifest_path; + /// Whether this file may be rewritten: small enough and no live delete file can apply to it. + bool is_candidate = false; +}; + +/// A live delete-file entry from a delete manifest. Bin-packing must not rewrite a data file +/// the delete could still apply to: rewriting changes the file's path and re-stamps its +/// sequence numbers, so the carried-forward delete would silently stop applying. +struct DeleteRecord +{ + FileContentType content_type; + /// Resolved data sequence number of the delete entry. + Int64 sequence_number; + Row partition_key; + /// Partition spec the delete manifest was written with. When it differs from the table's + /// default spec, the partition value is not comparable and the delete is treated as global. + Int32 partition_spec_id; + /// For position deletes / deletion vectors that reference exactly one data file. + std::optional lower_reference_data_file_path; + std::optional upper_reference_data_file_path; }; /// A bin: a group of small files from the same partition to be merged. struct Bin { Row partition_key; - std::vector files; + std::vector files; Int64 total_bytes = 0; Int64 total_records = 0; }; @@ -110,8 +131,13 @@ struct BinPackPlan { /// Bins of small files to merge. std::vector bins; - /// Manifest paths to carry forward unchanged (delete manifests + data manifests with no small files). + /// Manifest paths to carry forward unchanged (delete manifests, manifests written under a + /// non-default partition spec, and data manifests with no rewritten files). std::unordered_set carry_forward_manifest_paths; + /// Live data files from manifests that also contain rewritten files. They are not merged + /// (large files, single-file partitions, files excluded because of deletes) and stay in the + /// new snapshot as EXISTING entries, grouped by partition key. + std::unordered_map, PartitionKeyHash, PartitionKeyEqual> kept_groups; /// Total statistics across removed files. Int64 removed_data_files = 0; Int64 removed_records = 0; @@ -185,6 +211,15 @@ BinPackPlan buildBinPackPlan( plan.partition_spec = partition_spec; plan.partition_spec_id = partition_spec_id; + /// Spec ids with no partition fields; deletes written under them apply to every partition. + std::unordered_set unpartitioned_spec_ids; + for (UInt32 i = 0; i < partitions_specs->size(); ++i) + { + auto candidate = partitions_specs->getObject(static_cast(i)); + if (candidate->getArray(f_fields)->size() == 0) + unpartitioned_spec_ids.insert(candidate->getValue(f_spec_id)); + } + auto spec_fields = partition_spec->getArray(f_fields); std::vector partition_columns; for (UInt32 i = 0; i < spec_fields->size(); ++i) @@ -227,17 +262,47 @@ BinPackPlan buildBinPackPlan( IcebergPathFromMetadata::deserialize(current_manifest_list_path), log, secondary_storages); - /// Collect files per partition. - using PartitionFiles = std::vector; - std::unordered_map partition_files; - /// Track manifest paths with no small files to carry forward. - std::unordered_set manifests_with_only_large_files; + /// Every live data file, grouped by the manifest that lists it. + std::unordered_map> files_by_manifest; + /// Live delete entries, used to keep the rewrite away from files they can apply to. + std::vector delete_records; + size_t small_files_total = 0; for (const auto & manifest_file : manifest_list) { if (manifest_file.content_type == ManifestFileContentType::DELETE) { plan.carry_forward_manifest_paths.insert(manifest_file.manifest_file_path.serialize()); + + /// Record live delete entries so data files they can apply to are left out of the rewrite. + auto deletes_handle = getManifestFileEntriesHandle( + object_storage, persistent_table_components, context, log, + manifest_file, static_cast(current_schema_id), secondary_storages); + + for (const auto delete_content_type : {FileContentType::POSITION_DELETE, FileContentType::EQUALITY_DELETE}) + { + for (const auto & delete_file : deletes_handle.getFilesWithoutDeleted(delete_content_type)) + { + const auto & entry = delete_file->parsed_entry; + DeleteRecord record; + record.content_type = delete_content_type; + record.sequence_number = delete_file->sequence_number; + record.partition_key = entry->partition_key_value; + record.partition_spec_id = manifest_file.partition_spec_id; + record.lower_reference_data_file_path = entry->lower_reference_data_file_path; + record.upper_reference_data_file_path = entry->upper_reference_data_file_path; + delete_records.push_back(std::move(record)); + } + } + continue; + } + + if (manifest_file.partition_spec_id != partition_spec_id) + { + /// New manifests are written with the default partition spec and a single partition + /// tuple per manifest; entries written under an older spec would be recorded with + /// wrong partition values, so such manifests are carried forward unchanged. + plan.carry_forward_manifest_paths.insert(manifest_file.manifest_file_path.serialize()); continue; } @@ -245,109 +310,199 @@ BinPackPlan buildBinPackPlan( object_storage, persistent_table_components, context, log, manifest_file, static_cast(current_schema_id), secondary_storages); - bool has_small_files = false; + auto & manifest_records = files_by_manifest[manifest_file.manifest_file_path.serialize()]; for (const auto & data_file : files_handle.getFilesWithoutDeleted(FileContentType::DATA)) { const auto & entry = data_file->parsed_entry; - if (static_cast(entry->file_size_in_bytes) < min_file_size) + DataFileRecord record; + record.file_path = entry->file_path_key; + record.record_count = entry->record_count; + record.file_size_in_bytes = entry->file_size_in_bytes; + record.file_format = entry->file_format; + record.partition_key = entry->partition_key_value; + record.sort_order_id = entry->sort_order_id; + record.snapshot_id = entry->parsed_snapshot_id; + if (!record.snapshot_id.has_value()) + record.snapshot_id = manifest_file.added_snapshot_id; + record.sequence_number = entry->parsed_sequence_number; + if (!record.sequence_number.has_value()) + record.sequence_number = manifest_file.added_sequence_number; + record.file_sequence_number = entry->parsed_file_sequence_number; + if (!record.file_sequence_number.has_value()) + record.file_sequence_number = manifest_file.added_sequence_number; + + /// Carry over per-column stats. + for (const auto & [field_id, col_info] : entry->columns_infos) { - has_small_files = true; - SmallFileEntry small_entry; - small_entry.file_path = entry->file_path_key; - small_entry.record_count = entry->record_count; - small_entry.file_size_in_bytes = entry->file_size_in_bytes; - small_entry.file_format = entry->file_format; - small_entry.partition_key = entry->partition_key_value; - small_entry.sort_order_id = entry->sort_order_id; - small_entry.snapshot_id = entry->parsed_snapshot_id; - if (!small_entry.snapshot_id.has_value()) - small_entry.snapshot_id = manifest_file.added_snapshot_id; - small_entry.sequence_number = entry->parsed_sequence_number; - if (!small_entry.sequence_number.has_value()) - small_entry.sequence_number = manifest_file.added_sequence_number; - small_entry.file_sequence_number = entry->parsed_file_sequence_number; - if (!small_entry.file_sequence_number.has_value()) - small_entry.file_sequence_number = manifest_file.added_sequence_number; - - /// Carry over per-column stats. - for (const auto & [field_id, col_info] : entry->columns_infos) - { - if (col_info.bytes_size.has_value()) - small_entry.column_stats.column_sizes.emplace_back(field_id, *col_info.bytes_size); - if (col_info.rows_count.has_value()) - small_entry.column_stats.value_counts.emplace_back(field_id, *col_info.rows_count); - if (col_info.nulls_count.has_value()) - small_entry.column_stats.null_value_counts.emplace_back(field_id, *col_info.nulls_count); - } - for (const auto & [field_id, bounds] : entry->value_bounds) + if (col_info.bytes_size.has_value()) + record.column_stats.column_sizes.emplace_back(field_id, *col_info.bytes_size); + if (col_info.rows_count.has_value()) + record.column_stats.value_counts.emplace_back(field_id, *col_info.rows_count); + if (col_info.nulls_count.has_value()) + record.column_stats.null_value_counts.emplace_back(field_id, *col_info.nulls_count); + } + for (const auto & [field_id, bounds] : entry->value_bounds) + { + if (!bounds.first.isNull()) + record.column_stats.lower_bounds.emplace_back(field_id, bounds.first.safeGet()); + if (!bounds.second.isNull()) + record.column_stats.upper_bounds.emplace_back(field_id, bounds.second.safeGet()); + } + + record.source_manifest_path = manifest_file.manifest_file_path.serialize(); + record.is_candidate = static_cast(entry->file_size_in_bytes) < min_file_size; + if (record.is_candidate) + ++small_files_total; + manifest_records.push_back(std::move(record)); + } + } + + /// Exclude from the rewrite every small file that a live delete could still apply to, + /// following the Iceberg scan-planning rules: a position delete applies to data files with + /// data sequence number <= its own, an equality delete to those strictly lower. + size_t excluded_by_deletes = 0; + if (!delete_records.empty()) + { + for (auto & [manifest_path, records] : files_by_manifest) + { + for (auto & record : records) + { + if (!record.is_candidate) + continue; + + const Int64 data_sequence_number = record.sequence_number.value_or(0); + for (const auto & delete_record : delete_records) { - if (!bounds.first.isNull()) - small_entry.column_stats.lower_bounds.emplace_back(field_id, bounds.first.safeGet()); - if (!bounds.second.isNull()) - small_entry.column_stats.upper_bounds.emplace_back(field_id, bounds.second.safeGet()); + const bool sequence_applies = delete_record.content_type == FileContentType::EQUALITY_DELETE + ? data_sequence_number < delete_record.sequence_number + : data_sequence_number <= delete_record.sequence_number; + if (!sequence_applies) + continue; + + /// Partition match is checked only when the delete is partitioned under the + /// table's default spec. A delete under an unpartitioned spec is global, and + /// a delete under a different partitioned spec cannot be compared by + /// partition value; both are conservatively treated as matching. + if (!unpartitioned_spec_ids.contains(delete_record.partition_spec_id) + && delete_record.partition_spec_id == partition_spec_id + && !PartitionKeyEqual{}(delete_record.partition_key, record.partition_key)) + continue; + + /// A position delete or deletion vector naming exactly one data file applies + /// only to that file. + if (delete_record.content_type == FileContentType::POSITION_DELETE + && delete_record.lower_reference_data_file_path.has_value() + && delete_record.upper_reference_data_file_path.has_value() + && *delete_record.lower_reference_data_file_path == *delete_record.upper_reference_data_file_path + && *delete_record.lower_reference_data_file_path != record.file_path) + continue; + + record.is_candidate = false; + ++excluded_by_deletes; + break; } - - partition_files[entry->partition_key_value].push_back(std::move(small_entry)); } } - - if (!has_small_files) - manifests_with_only_large_files.insert(manifest_file.manifest_file_path.serialize()); + if (excluded_by_deletes > 0) + LOG_INFO(log, "Excluded {} small files from bin-packing because live delete files may apply to them", excluded_by_deletes); } - /// Carry forward manifests that only have large files. - for (const auto & path : manifests_with_only_large_files) - plan.carry_forward_manifest_paths.insert(path); + /// Group candidate files into bins per partition (by pointer; the records stay in + /// files_by_manifest until the plan is materialized below). + std::unordered_map, PartitionKeyHash, PartitionKeyEqual> partition_candidates; + for (const auto & [manifest_path, records] : files_by_manifest) + for (const auto & record : records) + if (record.is_candidate) + partition_candidates[record.partition_key].push_back(&record); - if (partition_files.empty()) + if (partition_candidates.empty()) { - LOG_INFO(log, "No small files found below threshold {} bytes; nothing to compact", min_file_size); + if (small_files_total == 0) + LOG_INFO(log, "No small files found below threshold {} bytes; nothing to compact", min_file_size); + else + LOG_INFO(log, "No files eligible for bin-packing; nothing to compact"); return plan; } - /// Group small files into bins per partition. - for (auto & [partition_key, files] : partition_files) + /// A bin: pointers to candidate files of one partition, up to `target_file_size` bytes. + struct BinPointers + { + Row partition_key; + std::vector files; + Int64 total_bytes = 0; + Int64 total_records = 0; + }; + std::vector bin_ptrs; + + for (auto & [partition_key, files] : partition_candidates) { /// Need at least 2 files to make compaction worthwhile. if (files.size() < 2) - { - /// Not enough files to merge — the manifest containing these is NOT carried forward as-is - /// because it also holds the small files. The existing compaction logic will write a - /// data manifest for the untouched partition in the new manifest list below. continue; - } - Bin current_bin; + BinPointers current_bin; current_bin.partition_key = partition_key; - for (auto & entry : files) + for (const auto * entry : files) { - if (current_bin.total_bytes + entry.file_size_in_bytes > static_cast(target_file_size) + if (current_bin.total_bytes + entry->file_size_in_bytes > static_cast(target_file_size) && !current_bin.files.empty()) { - plan.bins.push_back(std::move(current_bin)); - current_bin = Bin{}; + bin_ptrs.push_back(std::move(current_bin)); + current_bin = BinPointers{}; current_bin.partition_key = partition_key; } - current_bin.total_bytes += entry.file_size_in_bytes; - current_bin.total_records += entry.record_count; - plan.removed_data_files++; - plan.removed_records += entry.record_count; - plan.removed_files_size += entry.file_size_in_bytes; - current_bin.files.push_back(std::move(entry)); + current_bin.total_bytes += entry->file_size_in_bytes; + current_bin.total_records += entry->record_count; + current_bin.files.push_back(entry); } + /// A bin with a single file is not worth merging; the file stays in the snapshot. if (current_bin.files.size() >= 2) - plan.bins.push_back(std::move(current_bin)); - else + bin_ptrs.push_back(std::move(current_bin)); + } + + /// Materialize the plan: move binned records into bins, carry forward manifests with no + /// rewritten files, and keep every other file of a touched manifest as an EXISTING entry. + std::unordered_map bin_index_by_path; + for (size_t i = 0; i < bin_ptrs.size(); ++i) + for (const auto * entry : bin_ptrs[i].files) + bin_index_by_path.emplace(entry->file_path.serialize(), i); + + plan.bins.resize(bin_ptrs.size()); + for (size_t i = 0; i < bin_ptrs.size(); ++i) + { + plan.bins[i].partition_key = bin_ptrs[i].partition_key; + plan.bins[i].total_bytes = bin_ptrs[i].total_bytes; + plan.bins[i].total_records = bin_ptrs[i].total_records; + } + + for (auto & [manifest_path, records] : files_by_manifest) + { + const bool manifest_touched = std::any_of( + records.begin(), records.end(), + [&](const auto & record) { return bin_index_by_path.contains(record.file_path.serialize()); }); + + if (!manifest_touched) + { + plan.carry_forward_manifest_paths.insert(manifest_path); + continue; + } + + for (auto & record : records) { - /// Undo the stats for a single-file bin (not worth merging). - for (const auto & f : current_bin.files) + auto it = bin_index_by_path.find(record.file_path.serialize()); + if (it != bin_index_by_path.end()) + { + plan.removed_data_files++; + plan.removed_records += record.record_count; + plan.removed_files_size += record.file_size_in_bytes; + plan.bins[it->second].files.push_back(std::move(record)); + } + else { - plan.removed_data_files--; - plan.removed_records -= f.record_count; - plan.removed_files_size -= f.file_size_in_bytes; + plan.kept_groups[record.partition_key].push_back(std::move(record)); } } } @@ -603,104 +758,156 @@ bool executeBinPackCompaction( plan.num_partitions); /// Phase 3: Write manifest files. - /// We write two types of manifests: + /// We write three types of manifests: /// - Delete manifests: DELETED entries for old files (one per bin) /// - Add manifests: ADDED entries for new merged files (one per bin) + /// - Kept manifests: EXISTING entries for files of touched manifests that were not merged std::vector all_new_manifest_paths; std::vector all_manifest_sizes; std::vector all_existing_counts; std::vector all_entry_partition_spec_ids; + /// Write one data manifest for a set of entries sharing a partition key and record it + /// for the manifest list. `existing_counts` describes entries that already existed + /// (EXISTING or DELETED status); pass {0, 0, 0} for manifests of newly added files. + auto write_manifest_for_entries = [&]( + const Row & partition_key, + const std::vector & paths, + const std::vector & row_counts, + const std::vector & byte_counts, + const std::vector & formats, + const std::vector & stats, + const std::vector> & sort_order_ids, + const std::vector & lineage, + ManifestListEntryExistingCounts existing_counts) + { + auto manifest_path = generator.generateManifestEntryName(); + auto storage_path = path_resolver.resolve(manifest_path); + new_manifest_paths.push_back(manifest_path); + + auto buf = object_storage->writeObject( + StoredObject(storage_path), WriteMode::Rewrite, std::nullopt, + DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); + + generateManifestFile( + metadata_object, + plan.partition_columns, + partition_key, + plan.partition_types, + paths, + row_counts, + byte_counts, + std::nullopt, /// data_file_statistics + sample_block, + snapshot_result.snapshot, + write_format, + plan.partition_spec, + plan.partition_spec_id, + *buf, + FileContentType::DATA, + std::nullopt, /// user_defined_sequence_number + {}, /// per_file_stats + formats, + stats, + sort_order_ids, + lineage); + + buf->finalize(); + Int64 manifest_size = buf->count(); + if (manifest_size == 0) + manifest_size = object_storage->getObjectMetadata(storage_path, false).size_bytes; + + all_new_manifest_paths.push_back(manifest_path); + all_manifest_sizes.push_back(manifest_size); + all_existing_counts.push_back(existing_counts); + all_entry_partition_spec_ids.push_back(plan.partition_spec_id); + }; + for (auto & bin_result : bin_results) { /// Delete manifest: old files marked DELETED. - { - auto manifest_path = generator.generateManifestEntryName(); - auto storage_path = path_resolver.resolve(manifest_path); - new_manifest_paths.push_back(manifest_path); - - auto buf = object_storage->writeObject( - StoredObject(storage_path), WriteMode::Rewrite, std::nullopt, - DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); - - generateManifestFile( - metadata_object, - plan.partition_columns, - bin_result.partition_key, - plan.partition_types, - bin_result.old_file_paths, - bin_result.old_file_row_counts, - bin_result.old_file_byte_counts, - std::nullopt, /// data_file_statistics - sample_block, - snapshot_result.snapshot, - write_format, - plan.partition_spec, - plan.partition_spec_id, - *buf, - FileContentType::DATA, - std::nullopt, /// user_defined_sequence_number - {}, /// per_file_stats - bin_result.old_file_formats, - bin_result.old_file_stats, - bin_result.old_file_sort_order_ids, - bin_result.old_file_lineage); - - buf->finalize(); - Int64 manifest_size = buf->count(); - if (manifest_size == 0) - manifest_size = object_storage->getObjectMetadata(storage_path, false).size_bytes; - - all_new_manifest_paths.push_back(manifest_path); - all_manifest_sizes.push_back(manifest_size); - /// The DELETED manifest has existing (really: deleted) file counts for manifest-list accounting. - Int64 min_seq = std::numeric_limits::max(); - for (const auto & lineage : bin_result.old_file_lineage) - min_seq = std::min(min_seq, lineage.sequence_number.value_or(0)); - all_existing_counts.push_back( - {static_cast(bin_result.old_file_paths.size()), - static_cast(std::accumulate(bin_result.old_file_row_counts.begin(), bin_result.old_file_row_counts.end(), 0UL)), - min_seq}); - all_entry_partition_spec_ids.push_back(plan.partition_spec_id); - } + /// The DELETED manifest has existing (really: deleted) file counts for manifest-list accounting. + Int64 deleted_min_seq = std::numeric_limits::max(); + for (const auto & lineage : bin_result.old_file_lineage) + deleted_min_seq = std::min(deleted_min_seq, lineage.sequence_number.value_or(0)); + + write_manifest_for_entries( + bin_result.partition_key, + bin_result.old_file_paths, + bin_result.old_file_row_counts, + bin_result.old_file_byte_counts, + bin_result.old_file_formats, + bin_result.old_file_stats, + bin_result.old_file_sort_order_ids, + bin_result.old_file_lineage, + {static_cast(bin_result.old_file_paths.size()), + static_cast(std::accumulate(bin_result.old_file_row_counts.begin(), bin_result.old_file_row_counts.end(), 0UL)), + deleted_min_seq}); /// Add manifest: new merged files. + write_manifest_for_entries( + bin_result.partition_key, + bin_result.merged_file_paths, + bin_result.merged_file_row_counts, + bin_result.merged_file_byte_counts, + {}, /// formats + {}, /// stats + {}, /// sort_order_ids + {}, /// lineage + {0, 0, 0}); + } + + /// Kept manifests: live data files from touched manifests that were not merged (large + /// files, single-file partitions, files excluded because of deletes). They stay in the + /// new snapshot as EXISTING entries with their original snapshot id and sequence numbers. + for (const auto & [partition_key, kept_files] : plan.kept_groups) + { + std::vector kept_paths; + std::vector kept_row_counts; + std::vector kept_byte_counts; + std::vector kept_formats; + std::vector kept_stats; + std::vector> kept_sort_order_ids; + std::vector kept_lineage; + kept_paths.reserve(kept_files.size()); + kept_row_counts.reserve(kept_files.size()); + kept_byte_counts.reserve(kept_files.size()); + kept_formats.reserve(kept_files.size()); + kept_stats.reserve(kept_files.size()); + kept_sort_order_ids.reserve(kept_files.size()); + kept_lineage.reserve(kept_files.size()); + + Int64 kept_min_seq = std::numeric_limits::max(); + Int64 kept_total_rows = 0; + for (const auto & kept : kept_files) { - auto manifest_path = generator.generateManifestEntryName(); - auto storage_path = path_resolver.resolve(manifest_path); - new_manifest_paths.push_back(manifest_path); - - auto buf = object_storage->writeObject( - StoredObject(storage_path), WriteMode::Rewrite, std::nullopt, - DBMS_DEFAULT_BUFFER_SIZE, context->getWriteSettings()); - - generateManifestFile( - metadata_object, - plan.partition_columns, - bin_result.partition_key, - plan.partition_types, - bin_result.merged_file_paths, - bin_result.merged_file_row_counts, - bin_result.merged_file_byte_counts, - std::nullopt, /// data_file_statistics - sample_block, - snapshot_result.snapshot, - write_format, - plan.partition_spec, - plan.partition_spec_id, - *buf, - FileContentType::DATA); - - buf->finalize(); - Int64 manifest_size = buf->count(); - if (manifest_size == 0) - manifest_size = object_storage->getObjectMetadata(storage_path, false).size_bytes; - - all_new_manifest_paths.push_back(manifest_path); - all_manifest_sizes.push_back(manifest_size); - all_existing_counts.push_back({0, 0, 0}); /// New files: no existing counts. - all_entry_partition_spec_ids.push_back(plan.partition_spec_id); + kept_paths.push_back(kept.file_path); + kept_row_counts.push_back(static_cast(kept.record_count)); + kept_byte_counts.push_back(static_cast(kept.file_size_in_bytes)); + kept_formats.push_back(kept.file_format); + kept_stats.push_back(kept.column_stats); + kept_sort_order_ids.push_back(kept.sort_order_id); + + DataFileEntryLineage lineage; + lineage.added_snapshot_id = kept.snapshot_id; + lineage.sequence_number = kept.sequence_number; + lineage.file_sequence_number = kept.file_sequence_number; + kept_lineage.push_back(lineage); + + kept_min_seq = std::min(kept_min_seq, kept.sequence_number.value_or(0)); + kept_total_rows += kept.record_count; } + + write_manifest_for_entries( + partition_key, + kept_paths, + kept_row_counts, + kept_byte_counts, + kept_formats, + kept_stats, + kept_sort_order_ids, + kept_lineage, + {static_cast(kept_files.size()), kept_total_rows, kept_min_seq}); } /// Phase 4: Write manifest list. diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.cpp index 3414bf04e4c5..68db79b1b8ef 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.cpp @@ -74,7 +74,12 @@ void MultipleFileWriter::startNewFile() void MultipleFileWriter::consume(const Chunk & chunk) { - if (!current_file_num_rows || *current_file_num_rows >= max_data_file_num_rows || *current_file_num_bytes >= max_data_file_num_bytes) + /// A zero limit means "no limit": with a plain `>=` comparison it would roll over to a new + /// file after the first written chunk, splitting the output into one file per chunk. + const bool has_open_file = current_file_num_rows.has_value(); + const bool row_limit_reached = has_open_file && max_data_file_num_rows > 0 && *current_file_num_rows >= max_data_file_num_rows; + const bool bytes_limit_reached = has_open_file && max_data_file_num_bytes > 0 && *current_file_num_bytes >= max_data_file_num_bytes; + if (!has_open_file || row_limit_reached || bytes_limit_reached) { startNewFile(); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.h index 973b7e4932f3..536bf0f4555d 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MultipleFileWriter.h @@ -68,6 +68,7 @@ class MultipleFileWriter std::vector getDataFileEntries() const; private: + /// Rollover thresholds for the current data file; 0 means "no limit". UInt64 max_data_file_num_rows; UInt64 max_data_file_num_bytes; Poco::JSON::Array::Ptr schema; diff --git a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py index c5472a2b3caa..3ba6de59e13a 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py @@ -26,6 +26,16 @@ def _count_data_files(instance, table_name): return int(result) +def _get_data_file_sizes(instance, table_name): + """Return the sorted sizes of live DATA files via system.iceberg_files.""" + result = instance.query( + f"SELECT file_size_in_bytes FROM system.iceberg_files " + f"WHERE database = 'default' AND table = '{table_name}' AND content = 'DATA' " + f"ORDER BY file_size_in_bytes" + ).strip() + return [int(line) for line in result.splitlines()] if result else [] + + def _get_latest_snapshot_summary(instance, table_name): """Return the summary of the latest snapshot as a dict.""" raw = instance.query( @@ -179,3 +189,238 @@ def test_bin_pack_noop_when_no_small_files( assert files_after == files_before, ( f"Expected same number of files (nothing to compact): before={files_before}, after={files_after}" ) + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_keeps_single_file_last_bin( + started_cluster_iceberg_no_spark, format_version +): + """A bin that ends up with a single file must not drop that file from the snapshot.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_single_last_bin_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + num_batches = 5 + rows_per_batch = 100 + total_rows = num_batches * rows_per_batch + + for batch in range(num_batches): + values = ", ".join( + f"({batch * rows_per_batch + i}, 'row_{batch * rows_per_batch + i}')" + for i in range(rows_per_batch) + ) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + files_before = _count_data_files(instance, table_name) + assert files_before == num_batches + + data_before = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + + # Size the target so each bin holds at most 2 files: with 5 files that yields + # bins of 2, 2 and a leftover single file that must be kept, not dropped. + sizes = _get_data_file_sizes(instance, table_name) + assert len(sizes) == num_batches + target = 2 * sizes[-1] + 1 + assert 3 * sizes[0] > target, "File sizes vary too much for a deterministic bin split" + + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": target, + "iceberg_min_data_file_size_bytes": target, + }, + ) + + # Drop and recreate the table to pick up the new metadata. + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + ) + + # Two merged files plus the kept single file. + files_after = _count_data_files(instance, table_name) + assert files_after == 3, ( + f"Expected 3 files after compaction (2 merged + 1 kept), got {files_after}" + ) + + count_after = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_after == total_rows, ( + f"Row count mismatch: before={total_rows}, after={count_after}" + ) + + data_after = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + assert data_after == data_before, "Data mismatch after compaction" + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_keeps_single_file_partition( + started_cluster_iceberg_no_spark, format_version +): + """A partition with a single small file must keep that file in the new snapshot.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_single_partition_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, part String, value String)", + format_version=format_version, + partition_by="part", + ) + + # Partition 'a': three files. Partition 'b': one file. + for batch in range(3): + values = ", ".join( + f"({batch * 100 + i}, 'a', 'row_{batch * 100 + i}')" for i in range(100) + ) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + instance.query( + f"INSERT INTO {table_name} VALUES (10000, 'b', 'row_10000')", + settings={"allow_insert_into_iceberg": 1}, + ) + + files_before = _count_data_files(instance, table_name) + assert files_before == 4 + + data_before = instance.query( + f"SELECT id, part, value FROM {table_name} ORDER BY id" + ).strip() + + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + }, + ) + + # Drop and recreate the table to pick up the new metadata. + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + ) + + # Partition 'a' merged into one file; the single file of partition 'b' is kept. + files_after = _count_data_files(instance, table_name) + assert files_after == 2, ( + f"Expected 2 files after compaction (1 merged + 1 kept), got {files_after}" + ) + + count_after = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_after == 301, f"Row count mismatch: expected 301, got {count_after}" + + data_after = instance.query( + f"SELECT id, part, value FROM {table_name} ORDER BY id" + ).strip() + assert data_after == data_before, "Data mismatch after compaction" + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_preserves_position_deletes( + started_cluster_iceberg_no_spark, format_version +): + """Files referenced by live position deletes must not be rewritten: rewriting changes + the file path and re-stamps sequence numbers, so carried-forward deletes would stop + applying and deleted rows would resurface.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_deletes_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + num_batches = 4 + rows_per_batch = 100 + for batch in range(num_batches): + values = ", ".join( + f"({batch * rows_per_batch + i}, 'row_{batch * rows_per_batch + i}')" + for i in range(rows_per_batch) + ) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + # Delete rows from the first batch, producing a position-delete file. + instance.query( + f"DELETE FROM {table_name} WHERE id < 10", + settings={"allow_insert_into_iceberg": 1}, + ) + + expected_count = num_batches * rows_per_batch - 10 + count_before = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_before == expected_count + + data_before = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + + delete_files_before = int( + instance.query( + f"SELECT count() FROM system.iceberg_files " + f"WHERE database = 'default' AND table = '{table_name}' AND content = 'POSITION_DELETE'" + ).strip() + ) + assert delete_files_before > 0, "Expected a position-delete file after DELETE FROM" + + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + }, + ) + + # Drop and recreate the table to pick up the new metadata. + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + ) + + # Deleted rows must not resurface and no live row may be lost. + count_after = int(instance.query(f"SELECT count() FROM {table_name}").strip()) + assert count_after == expected_count, ( + f"Row count mismatch after compaction: expected {expected_count}, got {count_after}" + ) + + data_after = instance.query( + f"SELECT id, value FROM {table_name} ORDER BY id" + ).strip() + assert data_after == data_before, "Data mismatch after compaction" From 03e7d989a30a0814ca1a34bb23487b02bcbf7df3 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Mon, 28 Sep 2026 20:42:42 +0200 Subject: [PATCH 07/11] Address LLM comments --- .../DataLakes/Iceberg/BinPackRewrite.cpp | 328 ++++++++++++++---- .../DataLakes/Iceberg/BinPackRewrite.h | 29 +- .../DataLakes/Iceberg/IcebergMetadata.cpp | 72 ++-- .../DataLakes/Iceberg/IcebergWrites.cpp | 15 +- .../DataLakes/Iceberg/IcebergWrites.h | 6 +- .../DataLakes/Iceberg/MetadataGenerator.cpp | 18 +- .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 35 +- .../ObjectStorage/DataLakes/Iceberg/Utils.h | 20 +- .../Iceberg/tests/gtest_bin_pack_rewrite.cpp | 182 +++++++++- .../test_bin_pack_rewrite.py | 146 ++++++++ 10 files changed, 712 insertions(+), 139 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp index bf4e44b46945..b9dac42f6f59 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -5,7 +5,6 @@ #include #include -#include #include #include #include @@ -23,6 +22,7 @@ #include #include #include +#include #include #include @@ -31,6 +31,7 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; + extern const int CANNOT_WRITE_TO_FILE_BUFFER; extern const int LOGICAL_ERROR; extern const int ICEBERG_SPECIFICATION_VIOLATION; } @@ -59,6 +60,8 @@ struct DataFileRecord Int64 record_count; Int64 file_size_in_bytes; String file_format; + /// The schema the file was written with. + Int32 schema_id = 0; Row partition_key; std::optional sort_order_id; /// Lineage from the source manifest entry. @@ -267,6 +270,7 @@ BinPackPlan buildBinPackPlan( /// Live delete entries, used to keep the rewrite away from files they can apply to. std::vector delete_records; size_t small_files_total = 0; + size_t excluded_by_schema = 0; for (const auto & manifest_file : manifest_list) { @@ -319,6 +323,7 @@ BinPackPlan buildBinPackPlan( record.record_count = entry->record_count; record.file_size_in_bytes = entry->file_size_in_bytes; record.file_format = entry->file_format; + record.schema_id = data_file->resolved_schema_id; record.partition_key = entry->partition_key_value; record.sort_order_id = entry->sort_order_id; record.snapshot_id = entry->parsed_snapshot_id; @@ -351,12 +356,23 @@ BinPackPlan buildBinPackPlan( record.source_manifest_path = manifest_file.manifest_file_path.serialize(); record.is_candidate = static_cast(entry->file_size_in_bytes) < min_file_size; + /// Only Parquet columns are resolved by field id; other formats are read by name, + /// which is wrong for a file written with a different schema (e.g. before a rename). + if (record.is_candidate && record.schema_id != current_schema_id && Poco::toLower(record.file_format) != "parquet") + { + record.is_candidate = false; + ++excluded_by_schema; + } if (record.is_candidate) ++small_files_total; manifest_records.push_back(std::move(record)); } } + if (excluded_by_schema > 0) + LOG_INFO(log, "Excluded {} small non-Parquet files from bin-packing because they were written with an older schema", + excluded_by_schema); + /// Exclude from the rewrite every small file that a live delete could still apply to, /// following the Iceberg scan-planning rules: a position delete applies to data files with /// data sequence number <= its own, an equality delete to those strictly lower. @@ -440,27 +456,38 @@ BinPackPlan buildBinPackPlan( if (files.size() < 2) continue; + std::sort(files.begin(), files.end(), [](const DataFileRecord * lhs, const DataFileRecord * rhs) + { + if (lhs->file_size_in_bytes != rhs->file_size_in_bytes) + return lhs->file_size_in_bytes < rhs->file_size_in_bytes; + return lhs->file_path.serialize() < rhs->file_path.serialize(); + }); + BinPointers current_bin; current_bin.partition_key = partition_key; + /// Rewriting a single file only copies it to a new file of the same size, which would stay + /// a candidate and be rewritten again by every subsequent OPTIMIZE. Such a file stays as is. + auto flush_bin = [&] + { + if (current_bin.files.size() >= 2) + bin_ptrs.push_back(std::move(current_bin)); + current_bin = BinPointers{}; + current_bin.partition_key = partition_key; + }; + for (const auto * entry : files) { if (current_bin.total_bytes + entry->file_size_in_bytes > static_cast(target_file_size) && !current_bin.files.empty()) - { - bin_ptrs.push_back(std::move(current_bin)); - current_bin = BinPointers{}; - current_bin.partition_key = partition_key; - } + flush_bin(); current_bin.total_bytes += entry->file_size_in_bytes; current_bin.total_records += entry->record_count; current_bin.files.push_back(entry); } - /// A bin with a single file is not worth merging; the file stays in the snapshot. - if (current_bin.files.size() >= 2) - bin_ptrs.push_back(std::move(current_bin)); + flush_bin(); } /// Materialize the plan: move binned records into bins, carry forward manifests with no @@ -521,26 +548,19 @@ BinPackPlan buildBinPackPlan( return plan; } -} // anonymous namespace - +struct LatestMetadata +{ + Int32 version; + Poco::JSON::Object::Ptr object; +}; -bool executeBinPackCompaction( +LatestMetadata readLatestMetadata( const PersistentTableComponents & persistent_table_components, ObjectStoragePtr object_storage, - SecondaryStorages & secondary_storages, const DataLakeStorageSettings & data_lake_settings, - SharedHeader sample_block, ContextPtr context, - const String & write_format, - std::shared_ptr catalog, - const StorageID & table_id) + LoggerPtr log) { - LoggerPtr log = getLogger("IcebergBinPack"); - - const auto & settings = context->getSettingsRef(); - UInt64 target_size = settings[Setting::iceberg_target_data_file_size_bytes]; - UInt64 min_size = settings[Setting::iceberg_min_data_file_size_bytes]; - const auto [metadata_version, metadata_file_path, _] = getLatestOrExplicitMetadataFileAndVersion( object_storage, persistent_table_components.table_path, @@ -562,9 +582,120 @@ bool executeBinPackCompaction( persistent_table_components.metadata_compression_method, persistent_table_components.table_uuid); + return {metadata_version, metadata_object}; +} + +enum class MetadataFileOwner : uint8_t +{ + Absent, + Ours, + Other, +}; + +/// Who wrote the metadata file at `metadata_path`, judged by its current snapshot id. +MetadataFileOwner getMetadataFileOwner( + const IcebergPathFromMetadata & metadata_path, + CompressionMethod compression_method, + Int64 snapshot_id, + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + ContextPtr context, + LoggerPtr log) +{ + const auto storage_path = persistent_table_components.path_resolver.resolve(metadata_path); + if (!object_storage->exists(StoredObject(storage_path))) + return MetadataFileOwner::Absent; + + /// Bypass the metadata cache: the outcome of our own write is what is being checked. + auto metadata_object = getMetadataJSONObject( + storage_path, object_storage, /* metadata_cache */ nullptr, context, log, compression_method, std::nullopt); + const bool ours + = metadata_object->has(f_current_snapshot_id) && metadata_object->getValue(f_current_snapshot_id) == snapshot_id; + return ours ? MetadataFileOwner::Ours : MetadataFileOwner::Other; +} + +} // anonymous namespace + + +bool hasLivePositionDeletes( + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + const DataLakeStorageSettings & data_lake_settings, + ContextPtr context) +{ + LoggerPtr log = getLogger("IcebergBinPack"); + const auto metadata_object = readLatestMetadata(persistent_table_components, object_storage, data_lake_settings, context, log).object; + + if (!metadata_object->has(f_current_snapshot_id)) + return false; + const Int64 current_snapshot_id = metadata_object->getValue(f_current_snapshot_id); + if (current_snapshot_id < 0) + return false; + + String current_manifest_list_path; + auto snapshots = metadata_object->get(f_snapshots).extract(); + for (size_t i = 0; i < snapshots->size(); ++i) + { + const auto snapshot = snapshots->getObject(static_cast(i)); + if (snapshot->getValue(f_metadata_snapshot_id) == current_snapshot_id) + { + current_manifest_list_path = snapshot->getValue(f_manifest_list); + break; + } + } + if (current_manifest_list_path.empty()) + return false; + + const auto current_schema_id = metadata_object->getValue(f_current_schema_id); + auto schemas = metadata_object->getArray(f_schemas); + for (UInt32 i = 0; i < schemas->size(); ++i) + persistent_table_components.schema_processor->addIcebergTableSchema(schemas->getObject(i), context); + + auto manifest_list = getManifestList( + object_storage, persistent_table_components, context, + IcebergPathFromMetadata::deserialize(current_manifest_list_path), + log, secondary_storages); + + for (const auto & manifest_file : manifest_list) + { + if (manifest_file.content_type != ManifestFileContentType::DELETE) + continue; + auto deletes_handle = getManifestFileEntriesHandle( + object_storage, persistent_table_components, context, log, + manifest_file, current_schema_id, secondary_storages); + if (!deletes_handle.getFilesWithoutDeleted(FileContentType::POSITION_DELETE).empty()) + return true; + } + return false; +} + +BinPackCommitResult executeBinPackCompaction( + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + const DataLakeStorageSettings & data_lake_settings, + SharedHeader sample_block, + ContextPtr context, + const String & write_format) +{ + LoggerPtr log = getLogger("IcebergBinPack"); + + const auto & settings = context->getSettingsRef(); + UInt64 target_size = settings[Setting::iceberg_target_data_file_size_bytes]; + UInt64 min_size = settings[Setting::iceberg_min_data_file_size_bytes]; + + const auto [metadata_version, metadata_object] + = readLatestMetadata(persistent_table_components, object_storage, data_lake_settings, context, log); + + /// Format version 3 requires row lineage (`_row_id`, `_last_updated_sequence_number`) to be carried + /// into rewritten files, which the rewrite does not do yet. const Int32 format_version = metadata_object->getValue(f_format_version); - if (format_version < 2) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bin-packing compaction is supported only for Iceberg format_version >= 2"); + if (format_version != 2) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Bin-packing compaction is supported only for Iceberg format_version 2, got {}", + format_version); /// Build the plan. auto plan = buildBinPackPlan( @@ -574,7 +705,7 @@ bool executeBinPackCompaction( if (plan.bins.empty()) { LOG_INFO(log, "No bins to compact; table is already optimally packed"); - return true; + return BinPackCommitResult::Committed; } const auto & path_resolver = persistent_table_components.path_resolver; @@ -590,6 +721,7 @@ bool executeBinPackCompaction( std::vector new_data_file_paths; std::vector new_manifest_paths; IcebergPathFromMetadata manifest_list_path; + bool keep_files_on_error = false; auto cleanup = [&]() { @@ -628,6 +760,10 @@ bool executeBinPackCompaction( throw Exception(ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, "Missing schema for current-schema-id {}", current_schema_id); + /// Source files are read into the current schema; Parquet columns are matched by field id, + /// so renamed columns are found and dropped-then-re-added ones read as missing. + const ColumnMapperPtr current_schema_column_mapper = createColumnMapper(current_schema); + /// Phase 1: Read small files and write merged data files. /// For each bin, read all source files and write a merged file via MultipleFileWriter. Int64 total_added_files = 0; @@ -641,6 +777,7 @@ bool executeBinPackCompaction( std::vector merged_file_paths; std::vector merged_file_row_counts; std::vector merged_file_byte_counts; + std::vector merged_file_stats; /// The old files that were replaced. std::vector old_file_paths; std::vector old_file_row_counts; @@ -690,15 +827,19 @@ bool executeBinPackCompaction( auto parser_shared_resources = std::make_shared( settings, /*num_streams_=*/1); + const String source_format = file_entry.file_format.empty() ? write_format : file_entry.file_format; + const ColumnMapperPtr column_mapper + = Poco::toLower(source_format) == "parquet" ? current_schema_column_mapper : nullptr; + auto input_format = FormatFactory::instance().getInput( - file_entry.file_format.empty() ? write_format : file_entry.file_format, + source_format, *read_buffer, *sample_block, context, 8192, std::nullopt, /// format_settings parser_shared_resources, - std::make_shared(nullptr, context, nullptr, nullptr, nullptr), + std::make_shared(nullptr, context, column_mapper, nullptr, nullptr), true, /// is_remote_fs CompressionMethod::None, false); @@ -719,8 +860,9 @@ bool executeBinPackCompaction( result.old_file_sort_order_ids.push_back(file_entry.sort_order_id); result.old_file_stats.push_back(std::move(file_entry.column_stats)); + /// Per the spec, the `snapshot_id` of a DELETED entry is the snapshot that deleted the file, + /// so `added_snapshot_id` stays unset and the new `replace` snapshot id is written. DataFileEntryLineage lineage; - lineage.added_snapshot_id = file_entry.snapshot_id; lineage.sequence_number = file_entry.sequence_number; lineage.file_sequence_number = file_entry.file_sequence_number; lineage.status_override = ManifestEntryStatus::DELETED; @@ -732,6 +874,12 @@ bool executeBinPackCompaction( result.merged_file_paths = writer.getDataFiles(); result.merged_file_row_counts = writer.getDataFileRowCounts(); result.merged_file_byte_counts = writer.getDataFileByteCounts(); + result.merged_file_stats = writer.getPerFileStatistics(); + if (result.merged_file_stats.size() != result.merged_file_paths.size()) + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Bin-pack writer returned {} statistics for {} merged files", + result.merged_file_stats.size(), result.merged_file_paths.size()); for (size_t i = 0; i < result.merged_file_paths.size(); ++i) { @@ -768,8 +916,7 @@ bool executeBinPackCompaction( std::vector all_entry_partition_spec_ids; /// Write one data manifest for a set of entries sharing a partition key and record it - /// for the manifest list. `existing_counts` describes entries that already existed - /// (EXISTING or DELETED status); pass {0, 0, 0} for manifests of newly added files. + /// for the manifest list. `existing_counts` must match the statuses of the written entries. auto write_manifest_for_entries = [&]( const Row & partition_key, const std::vector & paths, @@ -779,6 +926,7 @@ bool executeBinPackCompaction( const std::vector & stats, const std::vector> & sort_order_ids, const std::vector & lineage, + const std::optional & data_file_statistics, ManifestListEntryExistingCounts existing_counts) { auto manifest_path = generator.generateManifestEntryName(); @@ -797,7 +945,7 @@ bool executeBinPackCompaction( paths, row_counts, byte_counts, - std::nullopt, /// data_file_statistics + data_file_statistics, sample_block, snapshot_result.snapshot, write_format, @@ -823,14 +971,21 @@ bool executeBinPackCompaction( all_entry_partition_spec_ids.push_back(plan.partition_spec_id); }; + const Int64 new_sequence_number = snapshot_result.snapshot->getValue(f_metadata_sequence_number); + for (auto & bin_result : bin_results) { /// Delete manifest: old files marked DELETED. - /// The DELETED manifest has existing (really: deleted) file counts for manifest-list accounting. Int64 deleted_min_seq = std::numeric_limits::max(); for (const auto & lineage : bin_result.old_file_lineage) deleted_min_seq = std::min(deleted_min_seq, lineage.sequence_number.value_or(0)); + ManifestListEntryExistingCounts deleted_counts; + deleted_counts.min_sequence_number = deleted_min_seq; + deleted_counts.deleted_files_count = static_cast(bin_result.old_file_paths.size()); + deleted_counts.deleted_rows_count = static_cast( + std::accumulate(bin_result.old_file_row_counts.begin(), bin_result.old_file_row_counts.end(), UInt64{0})); + write_manifest_for_entries( bin_result.partition_key, bin_result.old_file_paths, @@ -840,21 +995,34 @@ bool executeBinPackCompaction( bin_result.old_file_stats, bin_result.old_file_sort_order_ids, bin_result.old_file_lineage, - {static_cast(bin_result.old_file_paths.size()), - static_cast(std::accumulate(bin_result.old_file_row_counts.begin(), bin_result.old_file_row_counts.end(), 0UL)), - deleted_min_seq}); + std::nullopt, + deleted_counts); - /// Add manifest: new merged files. - write_manifest_for_entries( - bin_result.partition_key, - bin_result.merged_file_paths, - bin_result.merged_file_row_counts, - bin_result.merged_file_byte_counts, - {}, /// formats - {}, /// stats - {}, /// sort_order_ids - {}, /// lineage - {0, 0, 0}); + /// Add manifests: one per merged file, because `generateManifestFile` applies a single + /// statistics object to every entry of a manifest. + for (size_t i = 0; i < bin_result.merged_file_paths.size(); ++i) + { + std::optional merged_stats; + if (bin_result.merged_file_stats[i]) + merged_stats = *bin_result.merged_file_stats[i]; + + ManifestListEntryExistingCounts added_counts; + added_counts.min_sequence_number = new_sequence_number; + added_counts.added_files_count = 1; + added_counts.added_rows_count = static_cast(bin_result.merged_file_row_counts[i]); + + write_manifest_for_entries( + bin_result.partition_key, + {bin_result.merged_file_paths[i]}, + {bin_result.merged_file_row_counts[i]}, + {bin_result.merged_file_byte_counts[i]}, + {}, /// formats + {}, /// stats + {}, /// sort_order_ids + {}, /// lineage + merged_stats, + added_counts); + } } /// Kept manifests: live data files from touched manifests that were not merged (large @@ -907,6 +1075,7 @@ bool executeBinPackCompaction( kept_stats, kept_sort_order_ids, kept_lineage, + std::nullopt, {static_cast(kept_files.size()), kept_total_rows, kept_min_seq}); } @@ -947,32 +1116,58 @@ bool executeBinPackCompaction( auto hint_path = generator.generateVersionHint(); - const bool catalog_writes_metadata_file = catalog && catalog->isTransactional(); - if (!catalog_writes_metadata_file - && !writeMetadataFileAndVersionHint( - path_resolver, - generated_metadata_info, - json_representation, - hint_path, - object_storage, - context, - data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) + const auto commit_result = tryWriteMetadataFileAndVersionHint( + path_resolver, + generated_metadata_info, + json_representation, + hint_path, + object_storage, + context, + data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint]); + + if (commit_result == MetadataCommitResult::Conflict) { LOG_INFO(log, "Bin-pack commit conflict detected, cleaning up"); cleanup(); - return false; + return BinPackCommitResult::Conflict; } - if (catalog) + if (commit_result == MetadataCommitResult::Unknown) { - auto catalog_filename = path_resolver.resolveForCatalog(generated_metadata_info.path); - const auto & [namespace_name, table_name] = DataLake::parseTableName(table_id.getTableName()); - if (!catalog->updateMetadata(namespace_name, table_name, catalog_filename, snapshot_result.snapshot)) + /// From here on the new metadata may be live and reference the new files, + /// so they are removed only once another writer is known to own this version. + keep_files_on_error = true; + + const Int64 new_snapshot_id = snapshot_result.snapshot->getValue(f_metadata_snapshot_id); + MetadataFileOwner owner = MetadataFileOwner::Absent; + String verification_error; + try { - LOG_INFO(log, "Bin-pack commit conflict via catalog, cleaning up"); + owner = getMetadataFileOwner( + generated_metadata_info.path, generated_metadata_info.compression_method, new_snapshot_id, + persistent_table_components, object_storage, context, log); + } + catch (...) + { + verification_error = getCurrentExceptionMessage(false); + } + + if (!verification_error.empty() || owner == MetadataFileOwner::Absent) + throw Exception( + ErrorCodes::CANNOT_WRITE_TO_FILE_BUFFER, + "Outcome of writing Iceberg metadata file {} for bin-pack snapshot {} is unknown{}. " + "Files written by this attempt were left in place", + generated_metadata_info.path.serialize(), new_snapshot_id, + verification_error.empty() ? "" : fmt::format(" and could not be verified: {}", verification_error)); + + if (owner == MetadataFileOwner::Other) + { + LOG_INFO(log, "Bin-pack commit conflict detected after a failed metadata write, cleaning up"); + keep_files_on_error = false; cleanup(); - return false; + return BinPackCommitResult::Conflict; } + LOG_INFO(log, "Bin-pack metadata write reported an error, but snapshot {} is committed", new_snapshot_id); } } @@ -980,11 +1175,12 @@ bool executeBinPackCompaction( "{} old files removed ({} records, {} bytes)", plan.bins.size(), total_added_files, total_added_records, total_added_files_size, plan.removed_data_files, plan.removed_records, plan.removed_files_size); - return true; + return BinPackCommitResult::Committed; } catch (...) { - cleanup(); + if (!keep_files_on_error) + cleanup(); throw; } } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h index 5722ee7849db..64d7cb2e3e87 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h @@ -10,11 +10,6 @@ #include #include -namespace DataLake -{ -class ICatalog; -} - namespace DB { struct SecondaryStorages; @@ -23,6 +18,22 @@ struct SecondaryStorages; namespace DB::Iceberg { +enum class BinPackCommitResult : uint8_t +{ + Committed, + /// Another writer committed first; nothing written by this attempt is left behind, so it can be retried. + Conflict, +}; + +/// Whether the latest snapshot has live position delete files. Bin-packing leaves the files they +/// apply to untouched, so such tables are compacted by `compactIcebergTable` instead. +bool hasLivePositionDeletes( + const PersistentTableComponents & persistent_table_components, + ObjectStoragePtr object_storage, + SecondaryStorages & secondary_storages, + const DataLakeStorageSettings & data_lake_settings, + ContextPtr context); + /// Execute bin-packing compaction for an Iceberg table: merge small data files into /// larger ones, producing a `replace` snapshot that atomically swaps the old files /// for the merged results. Only data files smaller than `iceberg_min_data_file_size_bytes` @@ -30,17 +41,15 @@ namespace DB::Iceberg /// /// Leaves all other files, manifests, and snapshot history untouched. /// -/// Returns true on successful commit, false on commit conflict (caller should retry). -bool executeBinPackCompaction( +/// Throws, without removing the files it wrote, when the outcome of the metadata commit cannot be determined. +BinPackCommitResult executeBinPackCompaction( const PersistentTableComponents & persistent_table_components, ObjectStoragePtr object_storage, SecondaryStorages & secondary_storages, const DataLakeStorageSettings & data_lake_settings, SharedHeader sample_block, ContextPtr context, - const String & write_format, - std::shared_ptr catalog, - const StorageID & table_id); + const String & write_format); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index c9a0f61bbaa0..25a277c8c36c 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -68,6 +68,7 @@ #include #include #include +#include #include #include #include @@ -126,6 +127,8 @@ extern const int SUPPORT_IS_DISABLED; extern const int METADATA_MISMATCH; extern const int UNFINISHED; extern const int INCORRECT_DATA; +extern const int LIMIT_EXCEEDED; +extern const int QUERY_WAS_CANCELLED; } namespace Setting @@ -511,44 +514,71 @@ IcebergMetadata::getIcebergDataSnapshot(Poco::JSON::Object::Ptr metadata_object, } bool IcebergMetadata::optimize( - const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & /*format_settings*/) + const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & format_settings) { if (!context->getSettingsRef()[Setting::allow_experimental_iceberg_compaction]) throw Exception( ErrorCodes::BAD_ARGUMENTS, "Enable 'allow_experimental_iceberg_compaction' setting to call optimize for iceberg tables."); - static constexpr size_t MAX_BIN_PACK_RETRIES = 100; - const auto sample_block = std::make_shared(metadata_snapshot->getSampleBlock()); + auto invalidate_metadata_cache = [&]() + { + if (persistent_components.metadata_cache) + { + persistent_components.metadata_cache->remove(persistent_components.table_path); + if (persistent_components.table_uuid) + persistent_components.metadata_cache->remove(*persistent_components.table_uuid); + } + }; + + /// Bin-packing never rewrites a file a position delete applies to, so it cannot remove them. + if (Iceberg::hasLivePositionDeletes(persistent_components, object_storage, *secondary_storages, data_lake_settings, context)) + { + LOG_INFO(log, "Table has live position delete files, applying them with a full compaction"); + compactIcebergTable( + getHistory(context), + persistent_components, + object_storage, + secondary_storages, + data_lake_settings, + format_settings, + sample_block, + context, + write_format); + invalidate_metadata_cache(); + return true; + } + + /// Each attempt rewrites data files, so a conflict is retried only a few times. + static constexpr size_t MAX_BIN_PACK_RETRIES = 5; + for (size_t attempt = 0; attempt < MAX_BIN_PACK_RETRIES; ++attempt) { + if (auto elem = context->getProcessListElement(); elem && elem->isKilled()) + throw Exception(ErrorCodes::QUERY_WAS_CANCELLED, "OPTIMIZE TABLE cancelled during bin-pack retry loop"); + if (attempt > 0) LOG_INFO(log, "Retrying bin-pack compaction (attempt {}/{})", attempt + 1, MAX_BIN_PACK_RETRIES); - if (Iceberg::executeBinPackCompaction( - persistent_components, - object_storage, - *secondary_storages, - data_lake_settings, - sample_block, - context, - write_format, - /* catalog */ nullptr, - /* table_id */ StorageID::createEmpty())) + const auto result = Iceberg::executeBinPackCompaction( + persistent_components, + object_storage, + *secondary_storages, + data_lake_settings, + sample_block, + context, + write_format); + + if (result == Iceberg::BinPackCommitResult::Committed) { - if (persistent_components.metadata_cache) - { - persistent_components.metadata_cache->remove(persistent_components.table_path); - if (persistent_components.table_uuid) - persistent_components.metadata_cache->remove(*persistent_components.table_uuid); - } + invalidate_metadata_cache(); return true; } } - throw Exception(ErrorCodes::LOGICAL_ERROR, - "Bin-pack compaction failed to commit after {} attempts", MAX_BIN_PACK_RETRIES); + throw Exception(ErrorCodes::LIMIT_EXCEEDED, + "Bin-pack compaction did not commit after {} attempts because of concurrent modifications", MAX_BIN_PACK_RETRIES); } bool IcebergMetadata::optimizeManifestFiles( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp index 9c4fdff05a53..855b29f69d09 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp @@ -731,7 +731,8 @@ void generateManifestFile( else entry_status = ManifestEntryStatus::ADDED; manifest.field(Iceberg::f_status) = avro::GenericDatum(static_cast(entry_status)); - Int64 snapshot_id = (entry_lineage && entry_lineage->added_snapshot_id) + /// The spec defines `snapshot_id` of a DELETED entry as the snapshot that deleted the file. + Int64 snapshot_id = (entry_status != ManifestEntryStatus::DELETED && entry_lineage && entry_lineage->added_snapshot_id) ? *entry_lineage->added_snapshot_id : new_snapshot->getValue(Iceberg::f_metadata_snapshot_id); @@ -1151,14 +1152,14 @@ void generateManifestList( auto summary = new_snapshot->getObject(Iceberg::f_summary); if (manifest_only_rewrite) { - /// Manifest-only rewrite (`replace`): data files already existed, so they are reported as existing, not added. + /// Rewrite (`replace`): the caller supplies counts matching the entry statuses it wrote. const auto & counts = existing_entry_counts[entry_idx]; - setVersionedField(entry, 0, Iceberg::f_added_files_count); - setVersionedField(entry, counts.existing_files_count, Iceberg::f_existing_files_count); - setVersionedField(entry, 0, Iceberg::f_deleted_files_count); - setVersionedField(entry, 0, Iceberg::f_added_rows_count); + setVersionedField(entry, static_cast(counts.added_files_count), Iceberg::f_added_files_count); + setVersionedField(entry, static_cast(counts.existing_files_count), Iceberg::f_existing_files_count); + setVersionedField(entry, static_cast(counts.deleted_files_count), Iceberg::f_deleted_files_count); + setVersionedField(entry, counts.added_rows_count, Iceberg::f_added_rows_count); setVersionedField(entry, counts.existing_rows_count, Iceberg::f_existing_rows_count); - setVersionedField(entry, 0, Iceberg::f_deleted_rows_count); + setVersionedField(entry, counts.deleted_rows_count, Iceberg::f_deleted_rows_count); /// Recompute the `partitions` summary so pruning bounds survive the rewrite (lower_bound == upper_bound per field). if (!entry_partition_summaries.empty()) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h index 7c2004c11563..5f1d8232381e 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h @@ -133,13 +133,17 @@ void generateManifestFile( /// Optional schema to serialize into the manifest's Avro `schema` header; when null the table's current schema is used. Poco::JSON::Object::Ptr schema_to_serialize = nullptr); -/// Per manifest-list entry existing-file/existing-row counts for a manifest-only rewrite, where every referenced data file already existed. +/// Per manifest-list entry file/row counts for a rewrite whose manifests carry explicit entry statuses. struct ManifestListEntryExistingCounts { Int64 existing_files_count = 0; Int64 existing_rows_count = 0; /// Minimum data sequence number across the entries in this manifest, used as the manifest-list `min_sequence_number`. Int64 min_sequence_number = 0; + Int64 added_files_count = 0; + Int64 added_rows_count = 0; + Int64 deleted_files_count = 0; + Int64 deleted_rows_count = 0; }; void generateManifestList( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 327fe8347fc6..6baa020fdaf0 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -652,7 +652,13 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateReplaceSnapshot Int64 removed_files_size, Int64 num_partitions) { - int format_version = metadata_object->getValue(Iceberg::f_format_version); + /// A v3 `replace` must preserve row lineage of the rewritten rows, which is not implemented. + const Int32 format_version = metadata_object->getValue(Iceberg::f_format_version); + if (format_version != 2) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "A `replace` snapshot for bin-packing can be generated only for Iceberg format_version 2, got {}", + format_version); for (const auto * field : {Iceberg::f_metadata_log, Iceberg::f_snapshot_log}) if (!metadata_object->has(field)) @@ -711,16 +717,6 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateReplaceSnapshot new_snapshot->set(Iceberg::f_schema_id, metadata_object->getValue(Iceberg::f_current_schema_id)); new_snapshot->set(Iceberg::f_manifest_list, manifest_list_path.serialize()); - if (format_version >= 3) - { - Int64 next_row_id = metadata_object->has(Iceberg::f_next_row_id) && !metadata_object->isNull(Iceberg::f_next_row_id) - ? metadata_object->getValue(Iceberg::f_next_row_id) - : 0; - new_snapshot->set(Iceberg::f_first_row_id, next_row_id); - new_snapshot->set(Iceberg::f_added_rows, added_records); - metadata_object->set(Iceberg::f_next_row_id, next_row_id + added_records); - } - getOrCreateArray(metadata_object, Iceberg::f_snapshots)->add(new_snapshot); metadata_object->set(Iceberg::f_current_snapshot_id, snapshot_id); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index d378e21f03a5..d10f2905852a 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -298,14 +298,40 @@ bool writeMetadataFileAndVersionHint( DB::ObjectStoragePtr object_storage, DB::ContextPtr context, bool try_write_version_hint) +{ + return tryWriteMetadataFileAndVersionHint( + resolver, metadata_file_info, metadata_file_content, version_hint_path, object_storage, context, try_write_version_hint) + == MetadataCommitResult::Committed; +} + +MetadataCommitResult tryWriteMetadataFileAndVersionHint( + const IcebergPathResolver & resolver, + const GeneratedMetadataFileWithInfo & metadata_file_info, + const std::string & metadata_file_content, + const IcebergPathFromMetadata & version_hint_path, + DB::ObjectStoragePtr object_storage, + DB::ContextPtr context, + bool try_write_version_hint) { auto storage_metadata_path = resolver.resolve(metadata_file_info.path); auto storage_version_hint_path = resolver.resolve(version_hint_path); + + bool metadata_exists = false; try { - if (object_storage->exists(StoredObject(storage_metadata_path))) - return false; + metadata_exists = object_storage->exists(StoredObject(storage_metadata_path)); + } + catch (...) + { + /// Nothing has been written yet. + tryLogCurrentException(__PRETTY_FUNCTION__); + return MetadataCommitResult::Conflict; + } + if (metadata_exists) + return MetadataCommitResult::Conflict; + try + { Iceberg::writeMessageToFile( metadata_file_content, storage_metadata_path, @@ -317,8 +343,9 @@ bool writeMetadataFileAndVersionHint( } catch (...) { + /// Covers both a lost If-None-Match race and a write whose outcome is unknown (e.g. a timeout). tryLogCurrentException(__PRETTY_FUNCTION__); - return false; + return MetadataCommitResult::Unknown; } /// Once any writer has created `version-hint.text`, every subsequent writer must keep it in @@ -383,7 +410,7 @@ bool writeMetadataFileAndVersionHint( ++i; } - return true; + return MetadataCommitResult::Committed; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h index 4d403a785dc1..acd5e190e739 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h @@ -55,9 +55,27 @@ void writeMessageToFile( const std::string & write_if_match = "", DB::CompressionMethod compression_method = DB::CompressionMethod::None); +enum class MetadataCommitResult : uint8_t +{ + Committed, + /// The metadata file already existed, so nothing was written. + Conflict, + /// Writing the metadata file failed, and it is unknown whether the file was created. + Unknown, +}; + /// Tries to write metadata file and version hint file. Uses If-None-Match header to avoid overwriting existing files. -/// Maybe return false if failed to write metadata.json /// Will try to write hint multiple times, but will not report failure to write hint. +MetadataCommitResult tryWriteMetadataFileAndVersionHint( + const IcebergPathResolver & resolver, + const DB::GeneratedMetadataFileWithInfo & metadata_file_info, + const std::string & metadata_file_content, + const IcebergPathFromMetadata & version_hint_path, + DB::ObjectStoragePtr object_storage, + DB::ContextPtr context, + bool try_write_version_hint); + +/// Same as `tryWriteMetadataFileAndVersionHint`, returns false unless the result is `Committed`. bool writeMetadataFileAndVersionHint( const IcebergPathResolver & resolver, const DB::GeneratedMetadataFileWithInfo & metadata_file_info, diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp index d4dce4c20b95..54182aa5fb84 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_bin_pack_rewrite.cpp @@ -5,7 +5,14 @@ #include #include +#include +#include +#include +#include +#include #include +#include +#include #include #include #include @@ -77,6 +84,29 @@ Poco::JSON::Object::Ptr makeMetadataForReplace() return metadata; } +Poco::JSON::Object::Ptr makeReplaceSnapshot(Poco::JSON::Object::Ptr metadata) +{ + MetadataGenerator gen(metadata); + FileNamesGenerator file_gen("s3://bucket/table/", false, CompressionMethod::None, "Parquet"); + file_gen.setVersion(2); + auto metadata_path = file_gen.generateMetadataPathWithInfo(); + return gen.generateReplaceSnapshot(file_gen, metadata_path.path, 100, 1, 30, 300, 3, 30, 300, 1).snapshot; +} + +std::vector readAvroRecords(const String & data) +{ + ReadBufferFromString in(data); + auto reader_base = std::make_unique( + std::make_unique(in), MAX_AVRO_SCHEMA_DEPTH); + avro::DataFileReader reader(std::move(reader_base)); + + std::vector records; + avro::GenericDatum datum(reader.readerSchema()); + while (reader.read(datum)) + records.push_back(datum); + return records; +} + } @@ -163,29 +193,145 @@ TEST(IcebergBinPackRewrite, ReplaceSnapshotSequenceNumberIncremented) } -TEST(IcebergBinPackRewrite, DataFileEntryLineageStatusOverride) +TEST(IcebergBinPackRewrite, ReplaceSnapshotRejectsFormatVersion3) { - /// When status_override is set, the entry should use that status. - DataFileEntryLineage lineage; - lineage.added_snapshot_id = 42; - lineage.sequence_number = 1; - lineage.file_sequence_number = 1; - lineage.status_override = ManifestEntryStatus::DELETED; - - EXPECT_EQ(lineage.status_override.value(), ManifestEntryStatus::DELETED); + auto metadata = makeMetadataForReplace(); + metadata->set(f_format_version, 3); + MetadataGenerator gen(metadata); + + FileNamesGenerator file_gen("s3://bucket/table/", false, CompressionMethod::None, "Parquet"); + file_gen.setVersion(2); + auto metadata_path = file_gen.generateMetadataPathWithInfo(); + + EXPECT_THROW( + gen.generateReplaceSnapshot(file_gen, metadata_path.path, 100, 1, 10, 100, 2, 10, 100, 1), + DB::Exception); } -TEST(IcebergBinPackRewrite, DataFileEntryLineageNoOverrideDefaultsToExisting) +TEST(IcebergBinPackRewrite, ManifestEntriesStatusAndSnapshotId) { - /// When status_override is not set but lineage is present, the entry status - /// should be EXISTING (handled by generateManifestFile logic, but we test the struct). - DataFileEntryLineage lineage; - lineage.added_snapshot_id = 42; - lineage.sequence_number = 1; - lineage.file_sequence_number = 1; - - EXPECT_FALSE(lineage.status_override.has_value()); + auto metadata = makeMetadataForReplace(); + auto snapshot = makeReplaceSnapshot(metadata); + const Int64 new_snapshot_id = snapshot->getValue(f_metadata_snapshot_id); + const Int64 new_sequence_number = snapshot->getValue(f_metadata_sequence_number); + + DataFileEntryLineage deleted; + deleted.added_snapshot_id = 42; + deleted.sequence_number = 1; + deleted.file_sequence_number = 1; + deleted.status_override = ManifestEntryStatus::DELETED; + + DataFileEntryLineage existing; + existing.added_snapshot_id = 42; + existing.sequence_number = 1; + existing.file_sequence_number = 1; + + auto sample_block = std::make_shared( + Block{ColumnWithTypeAndName(std::make_shared(), "x")}); + + WriteBufferFromOwnString buf; + generateManifestFile( + metadata, + {}, + {}, + {}, + {IcebergPathFromMetadata::deserialize("s3://bucket/table/data/a.parquet"), + IcebergPathFromMetadata::deserialize("s3://bucket/table/data/b.parquet")}, + {10, 20}, + {100, 200}, + std::nullopt, + sample_block, + snapshot, + "PARQUET", + metadata->getArray(f_partition_specs)->getObject(0), + 0, + buf, + FileContentType::DATA, + std::nullopt, + {}, + {}, + {}, + {}, + {deleted, existing}); + buf.finalize(); + + auto entries = readAvroRecords(buf.str()); + ASSERT_EQ(entries.size(), 2); + + /// The spec defines `snapshot_id` of a DELETED entry as the snapshot that deleted the file. + const auto & deleted_entry = entries[0].value(); + EXPECT_EQ(deleted_entry.field(f_status).value(), static_cast(ManifestEntryStatus::DELETED)); + EXPECT_EQ(deleted_entry.field(f_snapshot_id).value(), new_snapshot_id); + EXPECT_EQ(deleted_entry.field(f_sequence_number).value(), 1); + + /// An EXISTING entry keeps the snapshot that added the file. + const auto & existing_entry = entries[1].value(); + EXPECT_EQ(existing_entry.field(f_status).value(), static_cast(ManifestEntryStatus::EXISTING)); + EXPECT_EQ(existing_entry.field(f_snapshot_id).value(), 42); + EXPECT_EQ(existing_entry.field(f_sequence_number).value(), 1); + EXPECT_NE(new_sequence_number, 1); +} + + +TEST(IcebergBinPackRewrite, ManifestListCountsMatchEntryStatuses) +{ + auto metadata = makeMetadataForReplace(); + auto snapshot = makeReplaceSnapshot(metadata); + const Int64 new_sequence_number = snapshot->getValue(f_metadata_sequence_number); + + ManifestListEntryExistingCounts deleted_counts; + deleted_counts.min_sequence_number = 1; + deleted_counts.deleted_files_count = 3; + deleted_counts.deleted_rows_count = 30; + + ManifestListEntryExistingCounts added_counts; + added_counts.min_sequence_number = new_sequence_number; + added_counts.added_files_count = 1; + added_counts.added_rows_count = 30; + + ManifestListEntryExistingCounts kept_counts; + kept_counts.min_sequence_number = 1; + kept_counts.existing_files_count = 2; + kept_counts.existing_rows_count = 50; + + IcebergPathResolver path_resolver("s3://bucket/table", "table"); + SecondaryStorages secondary_storages; + WriteBufferFromOwnString buf; + generateManifestList( + path_resolver, + metadata, + /* object_storage */ nullptr, + secondary_storages, + getContext().context, + {IcebergPathFromMetadata::deserialize("s3://bucket/table/metadata/deleted.avro"), + IcebergPathFromMetadata::deserialize("s3://bucket/table/metadata/added.avro"), + IcebergPathFromMetadata::deserialize("s3://bucket/table/metadata/kept.avro")}, + snapshot, + {1000, 1000, 1000}, + buf, + FileContentType::DATA, + /* use_previous_snapshots */ false, + {}, + {deleted_counts, added_counts, kept_counts}); + + auto entries = readAvroRecords(buf.str()); + ASSERT_EQ(entries.size(), 3); + + auto check = [](const avro::GenericDatum & datum, const ManifestListEntryExistingCounts & expected) + { + const auto & entry = datum.value(); + EXPECT_EQ(entry.field(f_added_files_count).value(), expected.added_files_count); + EXPECT_EQ(entry.field(f_existing_files_count).value(), expected.existing_files_count); + EXPECT_EQ(entry.field(f_deleted_files_count).value(), expected.deleted_files_count); + EXPECT_EQ(entry.field(f_added_rows_count).value(), expected.added_rows_count); + EXPECT_EQ(entry.field(f_existing_rows_count).value(), expected.existing_rows_count); + EXPECT_EQ(entry.field(f_deleted_rows_count).value(), expected.deleted_rows_count); + EXPECT_EQ(entry.field(f_min_sequence_number).value(), expected.min_sequence_number); + }; + check(entries[0], deleted_counts); + check(entries[1], added_counts); + check(entries[2], kept_counts); } diff --git a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py index 3ba6de59e13a..40f805c6261a 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py @@ -424,3 +424,149 @@ def test_bin_pack_preserves_position_deletes( f"SELECT id, value FROM {table_name} ORDER BY id" ).strip() assert data_after == data_before, "Data mismatch after compaction" + + # With live position deletes OPTIMIZE runs the full compaction, which applies them. + delete_files_after = int( + instance.query( + f"SELECT count() FROM system.iceberg_files " + f"WHERE database = 'default' AND table = '{table_name}' AND content = 'POSITION_DELETE'" + ).strip() + ) + assert delete_files_after == 0, ( + f"Expected position deletes to be applied by OPTIMIZE, {delete_files_after} delete files remain" + ) + + +def _count_snapshots(instance, table_name): + return int( + instance.query( + f"SELECT count() FROM system.iceberg_history " + f"WHERE database = 'default' AND table = '{table_name}'" + ).strip() + ) + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_converges(started_cluster_iceberg_no_spark, format_version): + """A second OPTIMIZE with the same settings must not rewrite anything: files that + cannot be combined with another file within the target size are left alone.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_converges_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + num_batches = 5 + rows_per_batch = 100 + for batch in range(num_batches): + values = ", ".join( + f"({batch * rows_per_batch + i}, 'row_{batch * rows_per_batch + i}')" + for i in range(rows_per_batch) + ) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + sizes = _get_data_file_sizes(instance, table_name) + target = 2 * sizes[-1] + 1 + optimize_settings = { + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": target, + "iceberg_min_data_file_size_bytes": target, + } + + instance.query(f"OPTIMIZE TABLE {table_name}", settings=optimize_settings) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + snapshots_after_first = _count_snapshots(instance, table_name) + data_after_first = instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() + + instance.query(f"OPTIMIZE TABLE {table_name}", settings=optimize_settings) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + assert _count_snapshots(instance, table_name) == snapshots_after_first, ( + "A repeated OPTIMIZE must not create a new snapshot" + ) + assert instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() == data_after_first + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_after_column_rename(started_cluster_iceberg_no_spark, format_version): + """Files written before a column rename must be read by field id, not by name, + otherwise the renamed column comes back empty in the merged file.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_rename_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value Nullable(String))", + format_version=format_version, + ) + + for batch in range(3): + values = ", ".join(f"({batch * 100 + i}, 'row_{batch * 100 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + instance.query(f"ALTER TABLE {table_name} RENAME COLUMN value TO label") + + values = ", ".join(f"({300 + i}, 'row_{300 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + data_before = instance.query(f"SELECT id, label FROM {table_name} ORDER BY id").strip() + assert "\\N" not in data_before + + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + }, + ) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + + assert _count_data_files(instance, table_name) == 1 + data_after = instance.query(f"SELECT id, label FROM {table_name} ORDER BY id").strip() + assert data_after == data_before, "Renamed column lost data after compaction" + + +def test_bin_pack_rejects_format_version_3(started_cluster_iceberg_no_spark): + """Bin-packing does not carry row lineage into rewritten files, so v3 tables are rejected.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_v3_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64)", + format_version=3, + ) + + error = instance.query_and_get_error( + f"OPTIMIZE TABLE {table_name}", + settings={"allow_experimental_iceberg_compaction": 1}, + ) + assert "BAD_ARGUMENTS" in error + assert "format_version 2" in error From c4c4578f6589ed36cd5716548291b35bc815629d Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Mon, 28 Sep 2026 22:21:02 +0200 Subject: [PATCH 08/11] Address LLM comments --- .../DataLakes/Iceberg/BinPackRewrite.cpp | 68 +++++-- .../DataLakes/Iceberg/BinPackRewrite.h | 2 +- .../DataLakes/Iceberg/Compaction.cpp | 36 +++- .../DataLakes/Iceberg/IcebergMetadata.cpp | 11 +- .../test_bin_pack_rewrite.py | 174 ++++++++++++++++++ 5 files changed, 263 insertions(+), 28 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp index b9dac42f6f59..6d9a14d8189d 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -9,6 +9,8 @@ #include #include #include +#include +#include #include #include #include @@ -34,6 +36,7 @@ namespace DB::ErrorCodes extern const int CANNOT_WRITE_TO_FILE_BUFFER; extern const int LOGICAL_ERROR; extern const int ICEBERG_SPECIFICATION_VIOLATION; + extern const int QUERY_WAS_CANCELLED; } namespace DB::Setting @@ -53,6 +56,14 @@ namespace DB::Iceberg namespace { +SharedHeader makeHeaderFromSchema(IcebergSchemaProcessor & schema_processor, Int32 schema_id) +{ + Block header; + for (const auto & column : *schema_processor.getClickhouseTableSchemaById(schema_id)) + header.insert(ColumnWithTypeAndName(column.type->createColumn(), column.type, column.name)); + return std::make_shared(std::move(header)); +} + /// A single live data file recorded from a data manifest. struct DataFileRecord { @@ -270,7 +281,6 @@ BinPackPlan buildBinPackPlan( /// Live delete entries, used to keep the rewrite away from files they can apply to. std::vector delete_records; size_t small_files_total = 0; - size_t excluded_by_schema = 0; for (const auto & manifest_file : manifest_list) { @@ -356,23 +366,12 @@ BinPackPlan buildBinPackPlan( record.source_manifest_path = manifest_file.manifest_file_path.serialize(); record.is_candidate = static_cast(entry->file_size_in_bytes) < min_file_size; - /// Only Parquet columns are resolved by field id; other formats are read by name, - /// which is wrong for a file written with a different schema (e.g. before a rename). - if (record.is_candidate && record.schema_id != current_schema_id && Poco::toLower(record.file_format) != "parquet") - { - record.is_candidate = false; - ++excluded_by_schema; - } if (record.is_candidate) ++small_files_total; manifest_records.push_back(std::move(record)); } } - if (excluded_by_schema > 0) - LOG_INFO(log, "Excluded {} small non-Parquet files from bin-packing because they were written with an older schema", - excluded_by_schema); - /// Exclude from the rewrite every small file that a live delete could still apply to, /// following the Iceberg scan-planning rules: a position delete applies to data files with /// data sequence number <= its own, an equality delete to those strictly lower. @@ -675,7 +674,6 @@ BinPackCommitResult executeBinPackCompaction( ObjectStoragePtr object_storage, SecondaryStorages & secondary_storages, const DataLakeStorageSettings & data_lake_settings, - SharedHeader sample_block, ContextPtr context, const String & write_format) { @@ -760,8 +758,12 @@ BinPackCommitResult executeBinPackCompaction( throw Exception(ErrorCodes::ICEBERG_SPECIFICATION_VIOLATION, "Missing schema for current-schema-id {}", current_schema_id); - /// Source files are read into the current schema; Parquet columns are matched by field id, - /// so renamed columns are found and dropped-then-re-added ones read as missing. + auto & schema_processor = *persistent_table_components.schema_processor; + const Int32 current_schema_id_int = static_cast(current_schema_id); + + /// The rewrite reads and writes the schema of the metadata it commits against, not the + /// storage's in-memory columns, which may be older than that metadata. + const SharedHeader sample_block = makeHeaderFromSchema(schema_processor, current_schema_id_int); const ColumnMapperPtr current_schema_column_mapper = createColumnMapper(current_schema); /// Phase 1: Read small files and write merged data files. @@ -828,13 +830,26 @@ BinPackCommitResult executeBinPackCompaction( settings, /*num_streams_=*/1); const String source_format = file_entry.file_format.empty() ? write_format : file_entry.file_format; - const ColumnMapperPtr column_mapper - = Poco::toLower(source_format) == "parquet" ? current_schema_column_mapper : nullptr; + const bool is_parquet = Poco::toLower(source_format) == "parquet"; + + /// Like `SELECT`, read a file with the schema it was written with (Parquet columns are + /// matched by field id when present, by name otherwise) and then evolve it to the + /// current schema, so renamed, retyped, added and dropped columns are handled. + SharedHeader read_header = sample_block; + ColumnMapperPtr column_mapper = is_parquet ? current_schema_column_mapper : nullptr; + std::shared_ptr schema_transform; + if (file_entry.schema_id != current_schema_id_int) + { + read_header = makeHeaderFromSchema(schema_processor, file_entry.schema_id); + column_mapper = is_parquet ? schema_processor.getColumnMapperById(file_entry.schema_id) : nullptr; + auto dag = schema_processor.getSchemaTransformationDagByIds(context, file_entry.schema_id, current_schema_id_int); + schema_transform = std::make_shared(dag->clone()); + } auto input_format = FormatFactory::instance().getInput( source_format, *read_buffer, - *sample_block, + *read_header, context, 8192, std::nullopt, /// format_settings @@ -846,9 +861,26 @@ BinPackCommitResult executeBinPackCompaction( while (true) { + if (auto elem = context->getProcessListElement(); elem && elem->isKilled()) + throw Exception(ErrorCodes::QUERY_WAS_CANCELLED, "OPTIMIZE TABLE cancelled during bin-pack rewrite"); + auto chunk = input_format->read(); if (chunk.empty()) break; + + if (schema_transform) + { + size_t num_rows = chunk.getNumRows(); + Block block = read_header->cloneWithColumns(chunk.detachColumns()); + schema_transform->execute(block, num_rows); + + Columns columns; + columns.reserve(sample_block->columns()); + for (const auto & column : *sample_block) + columns.push_back(block.getByName(column.name).column->convertToFullColumnIfConst()); + chunk = Chunk(std::move(columns), num_rows); + } + writer.consume(chunk); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h index 64d7cb2e3e87..d4469e5c3864 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.h @@ -38,6 +38,7 @@ bool hasLivePositionDeletes( /// larger ones, producing a `replace` snapshot that atomically swaps the old files /// for the merged results. Only data files smaller than `iceberg_min_data_file_size_bytes` /// are candidates; each bin targets `iceberg_target_data_file_size_bytes`. +/// Files are rewritten in the current schema of the latest metadata. /// /// Leaves all other files, manifests, and snapshot history untouched. /// @@ -47,7 +48,6 @@ BinPackCommitResult executeBinPackCompaction( ObjectStoragePtr object_storage, SecondaryStorages & secondary_storages, const DataLakeStorageSettings & data_lake_settings, - SharedHeader sample_block, ContextPtr context, const String & write_format); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp index 5855504b22ac..1897a531e6ce 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp @@ -958,20 +958,34 @@ namespace /// current compaction (OPTIME TABLE my_iceberg) supports only overwrites wich has only position delete files if (update.added_files == 0 && (update.added_position_deletes == update.added_delete_files) && update.added_position_deletes != 0) return std::nullopt; - [[fallthrough]]; - } - case SnapshotSummaryOperation::REPLACE: throw DB::Exception(ErrorCodes::NOT_IMPLEMENTED, "Unsupported snapshot's operation type {}", summary->getOperation()); + } + case SnapshotSummaryOperation::REPLACE: { + /// A `replace` can only be the first snapshot of the compacted history (see `compactIcebergTable`), + /// so it is regenerated as an append of all its live data files. + const auto totals = summary->getTotals(); + return SnapshotSummaryUpdateAppend{ + .added_files = totals.data_files, + .added_records = totals.records, + .added_files_size = totals.files_size, + .num_partitions = summary->getUpdate().num_partitions}; + } } }; -/// Current experimental compact implementation expects snapshots to be either appends or overwrites which has only position deletes -/// Lets force this invariant +/// Current experimental compact implementation expects snapshots to be either appends or overwrites which has only position deletes, +/// optionally preceded by a single `replace`. Lets force this invariant void checkIfIcebergHistorySupported(const IcebergHistory & history) { - for (const auto & history_record : history) + for (size_t i = 0; i < history.size(); ++i) { + const auto & history_record = history[i]; + if (i != 0 && history_record.snapshot_summary + && history_record.snapshot_summary->getOperation() == SnapshotSummaryOperation::REPLACE) + throw DB::Exception( + ErrorCodes::LOGICAL_ERROR, "A replace snapshot must start the compacted history, snapshot={}", history_record.snapshot_id); + auto append = tryGetAppendUpdate(history_record); if (append && append->added_files == 0) throw DB::Exception( @@ -1367,6 +1381,16 @@ void compactIcebergTable( ContextPtr context_, const String & write_format) { + /// Snapshots before the latest `replace` reference data files it removed. Compaction regenerates the + /// history from that snapshot on, which expires the older snapshots. + auto last_replace = std::find_if( + snapshots_info.rbegin(), + snapshots_info.rend(), + [](const Iceberg::IcebergHistoryRecord & record) + { return record.snapshot_summary && record.snapshot_summary->getOperation() == SnapshotSummaryOperation::REPLACE; }); + if (last_replace != snapshots_info.rend()) + snapshots_info.erase(snapshots_info.begin(), std::prev(last_replace.base())); + checkIfIcebergHistorySupported(snapshots_info); auto plan = getPlan( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 25a277c8c36c..49d0979e2c08 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -520,7 +520,13 @@ bool IcebergMetadata::optimize( throw Exception( ErrorCodes::BAD_ARGUMENTS, "Enable 'allow_experimental_iceberg_compaction' setting to call optimize for iceberg tables."); - const auto sample_block = std::make_shared(metadata_snapshot->getSampleBlock()); + /// Format version 3 requires row lineage to be carried into rewritten files, which neither + /// compaction path does yet. + if (persistent_components.format_version != 2) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "OPTIMIZE TABLE is supported only for Iceberg format_version 2, got {}", + persistent_components.format_version); auto invalidate_metadata_cache = [&]() { @@ -543,7 +549,7 @@ bool IcebergMetadata::optimize( secondary_storages, data_lake_settings, format_settings, - sample_block, + std::make_shared(metadata_snapshot->getSampleBlock()), context, write_format); invalidate_metadata_cache(); @@ -566,7 +572,6 @@ bool IcebergMetadata::optimize( object_storage, *secondary_storages, data_lake_settings, - sample_block, context, write_format); diff --git a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py index 40f805c6261a..64573a899c33 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py @@ -550,6 +550,180 @@ def test_bin_pack_after_column_rename(started_cluster_iceberg_no_spark, format_v assert data_after == data_before, "Renamed column lost data after compaction" +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_after_column_rename_read_by_name(started_cluster_iceberg_no_spark, format_version): + """ORC files carry no Iceberg field ids, so they are read by name (like Parquet files + written without field ids); files written before a rename must be read with their own + schema and then evolved, otherwise the renamed column comes back empty.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_rename_by_name_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value Nullable(String))", + format_version=format_version, + format="ORC", + ) + + for batch in range(3): + values = ", ".join(f"({batch * 100 + i}, 'row_{batch * 100 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + instance.query(f"ALTER TABLE {table_name} RENAME COLUMN value TO label") + + values = ", ".join(f"({300 + i}, 'row_{300 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + data_before = instance.query(f"SELECT id, label FROM {table_name} ORDER BY id").strip() + assert "\\N" not in data_before + + instance.query( + f"OPTIMIZE TABLE {table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + }, + ) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark, format="ORC") + + assert _count_data_files(instance, table_name) == 1 + data_after = instance.query(f"SELECT id, label FROM {table_name} ORDER BY id").strip() + assert data_after == data_before, "Renamed column lost data after compaction" + + +@pytest.mark.parametrize("format_version", [2]) +def test_bin_pack_through_stale_table_object(started_cluster_iceberg_no_spark, format_version): + """OPTIMIZE through a table object whose columns predate a schema change must rewrite + files in the latest schema, otherwise columns it does not know about are dropped.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_stale_" + get_uuid_str() + stale_table_name = table_name + "_stale" + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + for batch in range(3): + values = ", ".join(f"({batch * 100 + i}, 'row_{batch * 100 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + instance.query( + f"CREATE TABLE {stale_table_name} ENGINE=IcebergLocal(local, " + f"path = '/var/lib/clickhouse/user_files/iceberg_data/default/{table_name}/', format=Parquet)" + ) + + instance.query(f"ALTER TABLE {table_name} ADD COLUMN extra Nullable(String)") + values = ", ".join(f"({300 + i}, 'row_{300 + i}', 'extra_{300 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + data_before = instance.query(f"SELECT id, value, extra FROM {table_name} ORDER BY id").strip() + assert "extra_399" in data_before + + instance.query( + f"OPTIMIZE TABLE {stale_table_name}", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + }, + ) + + instance.query(f"DROP TABLE IF EXISTS {stale_table_name}") + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + + assert _count_data_files(instance, table_name) == 1 + data_after = instance.query(f"SELECT id, value, extra FROM {table_name} ORDER BY id").strip() + assert data_after == data_before, "Columns unknown to the stale table object were lost" + + +@pytest.mark.parametrize("format_version", [2]) +def test_optimize_after_bin_pack_and_delete(started_cluster_iceberg_no_spark, format_version): + """After a bin-pack `replace` snapshot, a DELETE routes the next OPTIMIZE to the full + compaction, which must accept the `replace` snapshot in the history.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_bin_pack_then_delete_" + get_uuid_str() + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=format_version, + ) + + for batch in range(4): + values = ", ".join(f"({batch * 100 + i}, 'row_{batch * 100 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + optimize_settings = { + "allow_experimental_iceberg_compaction": 1, + "iceberg_target_data_file_size_bytes": 10 * 1024 * 1024, + "iceberg_min_data_file_size_bytes": 10 * 1024 * 1024, + } + instance.query(f"OPTIMIZE TABLE {table_name}", settings=optimize_settings) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + assert _get_latest_snapshot_summary(instance, table_name).get("operation") == "replace" + + values = ", ".join(f"({400 + i}, 'row_{400 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + instance.query( + f"DELETE FROM {table_name} WHERE id < 10 OR id >= 490", + settings={"allow_insert_into_iceberg": 1}, + ) + + data_before = instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() + assert int(instance.query(f"SELECT count() FROM {table_name}").strip()) == 480 + + instance.query(f"OPTIMIZE TABLE {table_name}", settings=optimize_settings) + + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + + data_after = instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() + assert data_after == data_before, "Data mismatch after compacting a history with a replace snapshot" + + delete_files_after = int( + instance.query( + f"SELECT count() FROM system.iceberg_files " + f"WHERE database = 'default' AND table = '{table_name}' AND content = 'POSITION_DELETE'" + ).strip() + ) + assert delete_files_after == 0 + + def test_bin_pack_rejects_format_version_3(started_cluster_iceberg_no_spark): """Bin-packing does not carry row lineage into rewritten files, so v3 tables are rejected.""" instance = started_cluster_iceberg_no_spark.instances["node1"] From 14992f51ee983f27fe88b16d2bc3896168d0e7e6 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Mon, 28 Sep 2026 23:48:49 +0200 Subject: [PATCH 09/11] fix v3 guard for compaction --- .../DataLakes/Iceberg/Compaction.cpp | 7 +- .../DataLakes/Iceberg/IcebergMetadata.cpp | 3 +- .../test_bin_pack_rewrite.py | 65 +++++++++++++++++++ 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp index 1897a531e6ce..ce32fd1641bf 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp @@ -203,8 +203,11 @@ static Plan getPlan( Poco::JSON::Object::Ptr initial_metadata_object = getMetadataJSONObject(metadata_file_path, object_storage, persistent_table_components.metadata_cache, context, log, compression_method, persistent_table_components.table_uuid); - if (initial_metadata_object->getValue(Iceberg::f_format_version) < 2) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Compaction is supported only for format_version 2."); + /// The metadata is regenerated as format version 2 and row lineage is not carried over. + const Int32 format_version = initial_metadata_object->getValue(Iceberg::f_format_version); + if (format_version != 2) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, "Compaction is supported only for Iceberg format_version 2, got {}", format_version); auto current_schema_id = initial_metadata_object->getValue(Iceberg::f_current_schema_id); auto schemas = initial_metadata_object->getArray(Iceberg::f_schemas); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 49d0979e2c08..b7e0b240adbe 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -521,7 +521,8 @@ bool IcebergMetadata::optimize( ErrorCodes::BAD_ARGUMENTS, "Enable 'allow_experimental_iceberg_compaction' setting to call optimize for iceberg tables."); /// Format version 3 requires row lineage to be carried into rewritten files, which neither - /// compaction path does yet. + /// compaction path does yet. The cached version may be stale, so both paths check the latest + /// metadata again; this only rejects early. if (persistent_components.format_version != 2) throw Exception( ErrorCodes::BAD_ARGUMENTS, diff --git a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py index 64573a899c33..502b98498c02 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_bin_pack_rewrite.py @@ -744,3 +744,68 @@ def test_bin_pack_rejects_format_version_3(started_cluster_iceberg_no_spark): ) assert "BAD_ARGUMENTS" in error assert "format_version 2" in error + + +def test_optimize_rejects_table_upgraded_to_v3(started_cluster_iceberg_no_spark): + """A table upgraded to v3 by another engine after the table object was created must not be + compacted: the table object still caches format version 2, and the full compaction would + rewrite the metadata as version 2 and drop row lineage.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = "test_optimize_upgraded_v3_" + get_uuid_str() + table_dir = f"/var/lib/clickhouse/user_files/iceberg_data/default/{table_name}" + + create_iceberg_table( + "local", + instance, + table_name, + started_cluster_iceberg_no_spark, + "(id Int64, value String)", + format_version=2, + ) + + for batch in range(2): + values = ", ".join(f"({batch * 100 + i}, 'row_{batch * 100 + i}')" for i in range(100)) + instance.query( + f"INSERT INTO {table_name} VALUES {values}", + settings={"allow_insert_into_iceberg": 1}, + ) + + # A position delete routes OPTIMIZE to the full compaction. + instance.query( + f"DELETE FROM {table_name} WHERE id < 10", + settings={"allow_insert_into_iceberg": 1}, + ) + + data_before = instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() + list_data_files = f"ls -1 {table_dir}/data | sort" + data_files_before = instance.exec_in_container(["bash", "-c", list_data_files]) + + # Simulate an external upgrade: publish the next metadata version with format version 3. + fake_metadata = instance.exec_in_container( + [ + "bash", + "-c", + f"cd {table_dir}/metadata" + " && latest=$(ls -1 v*.metadata.json | sort -V | tail -n 1)" + " && next=$(( $(echo \"$latest\" | sed -E 's/^v([0-9]+).*/\\1/') + 1 ))" + " && sed -E 's/\"format-version\" *: *2/\"format-version\" : 3/' \"$latest\" > v$next.metadata.json" + " && grep -q '\"format-version\" : 3' v$next.metadata.json" + " && echo v$next.metadata.json", + ] + ).strip() + assert fake_metadata, "Failed to publish the upgraded metadata file" + + error = instance.query_and_get_error( + f"OPTIMIZE TABLE {table_name}", + settings={"allow_experimental_iceberg_compaction": 1}, + ) + assert "BAD_ARGUMENTS" in error + assert "format_version 2" in error + + # Nothing may have been rewritten or removed. + assert instance.exec_in_container(["bash", "-c", list_data_files]) == data_files_before + + instance.exec_in_container(["bash", "-c", f"rm {table_dir}/metadata/{fake_metadata}"]) + instance.query(f"DROP TABLE IF EXISTS {table_name}") + create_iceberg_table("local", instance, table_name, started_cluster_iceberg_no_spark) + assert instance.query(f"SELECT id, value FROM {table_name} ORDER BY id").strip() == data_before From ed57643a73a472b78adf9c1849381964d5eeb586 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 29 Sep 2026 05:13:24 +0200 Subject: [PATCH 10/11] Fix undefined `CANNOT_WRITE_TO_FILE_BUFFER` error code in Iceberg bin-pack rewrite The error code does not exist in `ErrorCodes.cpp`, which caused a link failure of `clickhouse` and `unit_tests_dbms`. Use `UNFINISHED` for the case where the outcome of the metadata commit is unknown. Co-authored-by: Cursor --- .../ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp index 6d9a14d8189d..f7db6e3a87f4 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -33,10 +33,10 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; - extern const int CANNOT_WRITE_TO_FILE_BUFFER; extern const int LOGICAL_ERROR; extern const int ICEBERG_SPECIFICATION_VIOLATION; extern const int QUERY_WAS_CANCELLED; + extern const int UNFINISHED; } namespace DB::Setting @@ -1186,7 +1186,7 @@ BinPackCommitResult executeBinPackCompaction( if (!verification_error.empty() || owner == MetadataFileOwner::Absent) throw Exception( - ErrorCodes::CANNOT_WRITE_TO_FILE_BUFFER, + ErrorCodes::UNFINISHED, "Outcome of writing Iceberg metadata file {} for bin-pack snapshot {} is unknown{}. " "Files written by this attempt were left in place", generated_metadata_info.path.serialize(), new_snapshot_id, From 9c9df0c2152811f3b1cd5ac510bda06d720e1568 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 29 Sep 2026 05:13:24 +0200 Subject: [PATCH 11/11] Fix undefined `CANNOT_WRITE_TO_FILE_BUFFER` error code in Iceberg bin-pack rewrite The error code does not exist in `ErrorCodes.cpp`, which caused a link failure of `clickhouse` and `unit_tests_dbms`. Use `UNFINISHED` for the case where the outcome of the metadata commit is unknown. --- .../ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp index 6d9a14d8189d..f7db6e3a87f4 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/BinPackRewrite.cpp @@ -33,10 +33,10 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; - extern const int CANNOT_WRITE_TO_FILE_BUFFER; extern const int LOGICAL_ERROR; extern const int ICEBERG_SPECIFICATION_VIOLATION; extern const int QUERY_WAS_CANCELLED; + extern const int UNFINISHED; } namespace DB::Setting @@ -1186,7 +1186,7 @@ BinPackCommitResult executeBinPackCompaction( if (!verification_error.empty() || owner == MetadataFileOwner::Absent) throw Exception( - ErrorCodes::CANNOT_WRITE_TO_FILE_BUFFER, + ErrorCodes::UNFINISHED, "Outcome of writing Iceberg metadata file {} for bin-pack snapshot {} is unknown{}. " "Files written by this attempt were left in place", generated_metadata_info.path.serialize(), new_snapshot_id,