Antalya-26:6 - iceberg v3: rewriteFiles - Added logic to merge small files into bins

[subkanthi]

Changelog category (leave one):

• New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Implement bin-packingReWriteFiles operation that Identifies small data files below a target size threshold(defined in Settings), groups them and merges to a large file, creates a snapshot that marks the old ones as DELETED and the new larger merge file as ADDED.

Leave all other files, manifests, and history untouched

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Unit tests
• Performance tests
• Aarch64 tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• CAS (content-addressed storage; Antalya only)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• OAuth (5m)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [efb67cd]

[subkanthi]

@blau-ai

[subkanthi]

Audit report: Altinity/ClickHouse#2420 (Iceberg bin-pack rewriteFiles via OPTIMIZE TABLE)

I found 2 High, 5 Medium and 3 Low confirmed defects. The two High ones are silent data problems: other engines can stop seeing the merged rows, and renamed columns can be overwritten with defaults.

I reviewed only the PR diff at head 1e415f2. I checked it against the base-branch helpers it calls: generateManifestFile, generateManifestList, writeMetadataFileAndVersionHint, compactIcebergTable and IcebergSchemaProcessor. To read those, I fetched the PR head into a local ref named tmp-pr2420. Nothing else in your repo changed.

AI audit note: This review was generated by AI.

Confirmed defects

High: the merged-files manifest reports zero files, so Spark, Trino and Flink can skip the compacted data

• Impact: After any successful OPTIMIZE, the manifest-list entry for the new merged files says added_files_count = 0 and existing_files_count = 0. Readers built on Iceberg Java skip data manifests that report no added and no existing files (ManifestGroup.ignoreDeleted, hasAddedFiles() || hasExistingFiles()). They also honour the DELETED entries for the old files, so every compacted row disappears for them. ClickHouse ignores these counts, which is why the integration tests pass.
• Anchor: BinPackRewrite.cpp / executeBinPackCompaction, the "Add manifest" call. The counts are written in IcebergWrites.cpp / generateManifestList, in the manifest_only_rewrite branch.
• Trigger: Any bin-pack commit, then a query from Spark or Trino.
• Why it's a defect: Every entry goes through the branch that hardcodes the added count to 0. It then copies {0, 0, 0} into the existing count. The manifest that holds the DELETED entries is also wrong: it reports them as existing_files_count.

// BinPackRewrite.cpp — new merged files
write_manifest_for_entries(..., bin_result.merged_file_paths, ..., {}, {0, 0, 0});
// IcebergWrites.cpp — manifest_only_rewrite = !existing_entry_counts.empty()
setVersionedField(entry, 0, Iceberg::f_added_files_count);
setVersionedField(entry, counts.existing_files_count, Iceberg::f_existing_files_count);

• Fix direction: Pass counts per manifest: added counts for the new-files manifest, and deleted counts plus deleted_rows_count for the DELETED manifest.
• Regression test: Read the compacted table with Spark (or pyiceberg in manifest-count mode) and assert the row count, and assert the counts in the manifest list.

High: source files are read by column name instead of field id, so renamed columns are rewritten as defaults

• Impact: Suppose a table has had RENAME COLUMN value TO v2. Files written before the rename contain value, but the reader looks for v2. Because input_format_parquet_allow_missing_columns defaults to true, the missing column is silently filled with default values. The merged file is then committed and the originals are marked DELETED, which permanently corrupts the data.
• Anchor: BinPackRewrite.cpp / executeBinPackCompaction, the FormatFactory::getInput(...) call.
• Trigger: ALTER TABLE ... RENAME COLUMN, then OPTIMIZE TABLE.
• Why it's a defect: The normal read path maps columns per file by field id through IcebergMetadata::getColumnMapperForObject. The rewrite path passes no column mapper:

auto input_format = FormatFactory::instance().getInput(fmt, *read_buffer, *sample_block, context, 8192,
    std::nullopt, parser_shared_resources,
    std::make_shared<FormatFilterInfo>(nullptr, context, nullptr, nullptr, nullptr), ...);

• Fix direction: Build a column mapper from each source file's schema id, the same way the normal read path does, or refuse to rewrite files whose schema id differs from the current one.
• Regression test: Insert rows, rename a column, insert more rows, run OPTIMIZE, and assert the old rows keep their values.

Medium: the greedy binning emits single-file bins mid-loop, so compaction never converges with default settings

• Impact: When a bin overflows, it is pushed even if it holds one file; the >= 2 check only applies to the last bin. With the defaults (candidates are below 384 MiB, target is 512 MiB), files between about 257 and 384 MiB are each rewritten alone into a file of the same size. That file is still a candidate next time, so every OPTIMIZE rewrites them again.
• Anchor: BinPackRewrite.cpp / buildBinPackPlan, the bin loop.

if (current_bin.total_bytes + entry->file_size_in_bytes > target && !current_bin.files.empty())
{
    bin_ptrs.push_back(std::move(current_bin));   // may hold exactly 1 file

• Trigger: One partition with three files of about 300 MiB each.
• Why it's a defect: It causes unbounded, repeated write amplification with no reduction in file count.
• Fix direction: Apply the files.size() >= 2 check to every bin before pushing it, and sort or pair files by size first.
• Regression test: Three files each between half the target and the minimum size; assert the file count and that a second OPTIMIZE is a no-op.

Medium: DELETED entries keep the snapshot id that added the file

• Impact: The Iceberg spec defines an entry's snapshot_id as the snapshot that deleted the file when the status is DELETED. Here it carries the original add snapshot. Incremental and changelog scans, and file cleanup during snapshot expiry, will attribute the removal to the wrong snapshot.
• Anchor: BinPackRewrite.cpp, the old-file lineage. IcebergWrites.cpp / generateManifestFile writes lineage->added_snapshot_id whenever it is set.

lineage.added_snapshot_id = file_entry.snapshot_id;      // original add snapshot
lineage.status_override = ManifestEntryStatus::DELETED;

• Trigger: Any bin-pack commit.
• Fix direction: Leave added_snapshot_id unset for DELETED entries so the new snapshot id is used.
• Regression test: Parse the new manifest and assert that DELETED entries carry the replace snapshot's id.

Medium: format-version-3 tables are accepted, but row lineage is not preserved

• Impact: Merged rows get fresh _row_id values. The snapshot's first_row_id is moved forward by added_records, and the new files contain no _row_id or _last_updated_sequence_number columns. The spec requires a rewrite to keep row ids stable. The existing manifest-only compaction explicitly rejects format version 3; this path only rejects versions below 2.
• Anchor: executeBinPackCompaction (if (format_version < 2)) and the format-version-3 block in MetadataGenerator::generateReplaceSnapshot.
• Trigger: OPTIMIZE on a format-version-3 table.
• Fix direction: Reject format version 3 until lineage columns are copied into the new files.
• Regression test: On a format-version-3 table, assert _row_id values are identical before and after OPTIMIZE.

Medium: OPTIMIZE no longer purges position deletes, and the old compaction path is now dead code

• Impact: optimize used to call compactIcebergTable, which applied position deletes and was triggered precisely when position deletes existed. It now only bin-packs, and it deliberately skips files that deletes apply to. Merge-on-read deletes can no longer be compacted away, compactIcebergTable has no callers left, and format_settings is silently ignored.
• Anchor: IcebergMetadata.cpp / IcebergMetadata::optimize.
• Trigger: DELETE FROM, then OPTIMIZE: the delete files stay, and the affected files are never compacted.
• Why it's a defect: It is a user-visible functional regression of an existing command.
• Fix direction: Keep delete-applying compaction, either as a separate mode or by running it first, or document the removal explicitly.
• Regression test: After DELETE and OPTIMIZE, assert the position-delete file count, whichever behaviour is chosen.

Medium: an ambiguous commit failure triggers destructive cleanup

• Impact: writeMetadataFileAndVersionHint returns false for any exception from the metadata write, not only for "file exists". Bin-pack treats false as a conflict and deletes the new data files, manifests and manifest list. If the write actually succeeded (for example, a timeout followed by a retry that gets 412 Precondition Failed), the committed metadata now points at deleted files.
• Anchor: BinPackRewrite.cpp, phase 5 (cleanup(); return false;). The underlying catch-all is in Utils.cpp / writeMetadataFileAndVersionHint.
• Trigger: A network error or retry during the metadata PUT that uses If-None-Match.
• Why it's a defect: Deleting files after an ambiguous outcome is fail-open and destructive.
• Fix direction: Only clean up on a definite "file exists" result. On other errors, re-read the metadata to find out whether the commit happened before deleting anything.
• Regression test: Inject a failure after the metadata object has been written, and assert the table is still readable.

Low: the retry loop can repeat a full rewrite 100 times, then throws LOGICAL_ERROR for an ordinary conflict

• Impact: Each retry re-reads and rewrites every candidate file, with no backoff. Under steady concurrent inserts, one OPTIMIZE can do 100 full rewrites. It then throws LOGICAL_ERROR, which is the wrong error code for an environmental condition and aborts debug and sanitizer builds.
• Anchor: IcebergMetadata::optimize (MAX_BIN_PACK_RETRIES = 100).
• Fix direction: Use a small retry limit with backoff, and a conflict-specific error code.
• Regression test: Force repeated conflicts and assert the error code and the number of attempts.

Low: merged files are written without column statistics

• Impact: The new-files manifest gets no bounds or counts, so min/max pruning is lost for compacted data. This is a performance regression, not a correctness problem.
• Anchor: BinPackRewrite.cpp, the "Add manifest" call (std::nullopt /* data_file_statistics */, {} /* stats */).
• Fix direction: Pass MultipleFileWriter's per-file statistics through.
• Regression test: Assert that system.iceberg_files shows bounds for the merged files.

Low: the tests do not exercise the risky paths

• Impact: DataFileEntryLineageStatusOverride and DataFileEntryLineageNoOverrideDefaultsToExisting only check struct fields the test itself just set. No test covers the status_override path in generateManifestFile, the manifest-list counts, renamed columns, format version 3, or reads by another engine. That is why neither High defect is caught.
• Anchor: gtest_bin_pack_rewrite.cpp, test_bin_pack_rewrite.py.
• Fix direction: Add the regression tests listed above, including one that parses the manifests.

Coverage summary

• Scope reviewed: all 14 changed files; the base helpers were checked only where the new code calls them.
• Categories failed: manifest and spec encoding, schema evolution, planning and bin logic, format-version-3 compatibility, behaviour regression, commit and rollback, retry/resource use, test adequacy.
• Categories passed:
  • Delete-applicability exclusion rules (sequence numbers, partition, single-file references).
  • Carry-forward of manifests with a different spec or with deletes.
  • Kept-file grouping.
  • Snapshot totals.
  • MultipleFileWriter zero-limit fix.
  • Settings and settings-history wiring.
  • Shared state: addIcebergTableSchema holds the processor mutex, and current_schema_id is only used transiently.
  • Integer and signedness conversions.
• Not confirmed, excluded from the defects above:
  • The catalog commit branch writes metadata before the catalog update and then cleans up if the catalog rejects it. optimize always passes catalog = nullptr, so this branch can't currently run.
  • PartitionKeyHash combines field hashes with XOR, which only affects performance through collisions.
  • Whether a long rewrite can be cancelled with KILL QUERY.
• Assumptions and limits: This was static analysis only; I built and ran nothing. The Spark/Trino skip in the first High finding relies on Iceberg Java's ManifestGroup and GenericManifestFile behaviour, not on a runtime check. Confidence is Medium-High overall and High for the ClickHouse-side code paths. Running Spark against a compacted table, and a rename-then-OPTIMIZE test, would confirm both High findings.

[subkanthi]

Audit report: #2420 at c4c4578

AI audit note: This review was generated by AI.

Audit update for PR #2420 (Iceberg bin-pack rewriteFiles via OPTIMIZE TABLE), head c4c4578, diff against antalya-26.6.

All 10 defects from the previous audit (head 1e415f2) are fixed in 03e7d98 and c4c4578. One Medium and three Low defects remain, listed below.

Confirmed defects

Medium: the v3 guard uses the format version cached when the table object was created, and full compaction still accepts v3 and writes v2 metadata

• Impact: Suppose the table is upgraded to v3 after the ClickHouse table object was created, for example by Spark with format-version=3. OPTIMIZE with live deletes then passes the guard and runs full compaction. That regenerates the metadata as format version 2, which is a forbidden downgrade, and discards row lineage. The bin-pack branch correctly rejects the same table, because it re-reads the latest metadata.
• Anchor: IcebergMetadata::optimize, the persistent_components.format_version check. Downstream, Compaction.cpp / getPlan checks < 2, and createEmptyMetadataFile defaults to 2.
• Trigger: The ClickHouse table object is created on v2, an external engine upgrades the table to v3 and adds deletes, and then OPTIMIZE runs.

if (persistent_components.format_version != 2)          // set once in the IcebergMetadata constructor
    throw Exception(ErrorCodes::BAD_ARGUMENTS, "OPTIMIZE TABLE is supported only for Iceberg format_version 2, got {}", ...);

// Compaction.cpp / getPlan (latest metadata)
if (initial_metadata_object->getValue<Int32>(Iceberg::f_format_version) < 2)   // v3 passes

• Why defect: The PR's own comment says neither compaction path supports v3, but the guard enforces that only against stale state.
• Fix direction: Check the format version in getPlan against the latest metadata (!= 2), or read the latest version in optimize.
• Regression test direction: Upgrade the metadata to v3 after the table is attached, add a delete, and assert that OPTIMIZE fails with BAD_ARGUMENTS.

Low: after full compaction, the regenerated replace snapshot points to a parent that no longer exists

• Impact: compactIcebergTable erases every snapshot before the last replace. The regenerated first snapshot still records the erased snapshot as its parent-snapshot-id. ClickHouse tolerates this, because getHistory walks parents with std::map::operator[] and stops. Other engines' ancestry-based operations (incremental reads, rollback, cherrypick validation) see a broken lineage.
• Anchor: Compaction.cpp / compactIcebergTable, the truncation step, and writeMetadataFiles, which passes history_record.parent_id to generateNextMetadata.

snapshots_info.erase(snapshots_info.begin(), std::prev(last_replace.base()));
...
metadata_generator.generateNextMetadata(plan.generator, generated_metadata_info.path,
    history_record.parent_id, ...);   // parent no longer present in the new metadata

• Fix direction: Pass "no parent" for the first record of the truncated history.
• Regression test direction: After bin-pack, DELETE, and OPTIMIZE, assert that the first snapshot in the new metadata has no parent-snapshot-id, or one that exists.

Low: the "outcome unknown, but actually committed" path skips the version-hint update

• Impact: When the metadata write throws but verification shows our snapshot landed, bin-pack reports success. However, tryWriteMetadataFileAndVersionHint returned before its version-hint loop. Readers with iceberg_use_version_hint = 1 keep seeing the pre-compaction version until the next writer. The data they read is stale but correct, because the old files are only marked DELETED.
• Anchor: Utils.cpp / tryWriteMetadataFileAndVersionHint, and BinPackRewrite.cpp / executeBinPackCompaction phase 5.

catch (...)
{
    tryLogCurrentException(__PRETTY_FUNCTION__);
    return MetadataCommitResult::Unknown;   // version-hint loop below is skipped
}

• Fix direction: Once the snapshot is confirmed ours, run the version-hint update. For example, factor out the hint loop and call it from the verified-committed branch.
• Regression test direction: A unit test with a fault-injecting object storage that fails the metadata PUT after writing it; assert that the hint equals the new version.

Low: the new failure-handling paths have no tests

• Impact: Nothing exercises these paths:
  • the Unknown commit outcome, including the Absent, Other and Ours owners and keeping files on error;
  • the history truncation in compactIcebergTable when there is more than one replace;
  • the new hasLivePositionDeletes dispatch when there are equality deletes only.
• CI: The current CI run is a draft run with 84 of 131 checks skipped. The gtests and the 9 integration tests have no recorded result for c4c4578.
• Anchor: gtest_bin_pack_rewrite.cpp, test_bin_pack_rewrite.py.
• Fix direction: Add a gtest for getMetadataFileOwner and the commit branches using a mocked IObjectStorage, add an integration test with two consecutive bin-packs followed by DELETE and OPTIMIZE, and run the non-draft CI.

Coverage summary

• Scope reviewed: All 17 changed files at c4c4578. For the previous audit's findings I verified each fix; everything added in 03e7d98 and c4c4578 got a full review: schema-evolution reads, the tri-state commit, the optimize dispatch, and Compaction.cpp accepting replace.
• Previously reported, now fixed:
  • H1: manifest-list counts.
  • H2: reads by field id or name with a schema-transform DAG.
  • M1: binning convergence.
  • M2: DELETED entries' snapshot_id.
  • M3: v3 rejection in bin-pack.
  • M4: position deletes routed to full compaction.
  • M5: no destructive cleanup when the commit outcome is unknown.
  • L1: 5 retries, a kill check, and LIMIT_EXCEEDED.
  • L2: per-file statistics.
  • L3: tautological gtests replaced.
• Categories failed: format-version gating, snapshot lineage after truncation, commit side effects (the version hint), and test adequacy.
• Categories passed:
  • bin planning and the single-file rule;
  • the delete-applicability exclusions;
  • carry-forward and kept manifests;
  • manifest and manifest-list counts, including the Avro int narrowing;
  • DELETED and EXISTING lineage;
  • schema-transform reads for Parquet (field id) and ORC/Avro (name);
  • cancellation and retry bounds;
  • cleanup ownership (keep_files_on_error);
  • SecondaryStorages and schema-processor locking;
  • integer and signedness handling.
• Not confirmed or pre-existing, excluded above:
  • Full compaction deletes everything under data/ and metadata/ that its history does not reference, and it has no commit-conflict check. A concurrent INSERT during a delete-triggered OPTIMIZE could lose data. This predates the PR, but the PR routes to it again.
  • Int32 total_records_count in writeMetadataFiles can overflow for tables above 2^31 rows. This also predates the PR.
• Assumptions and limits: This was static analysis only; nothing was built or run for c4c4578. The build attempted in the previous session aborted before completion. Confidence is High for the ClickHouse code paths and Medium for the cross-engine effects of the dangling parent.