ObjectIteratorSplitByBuckets::next copied ObjectInfo by value and wrapped it in std::make_shared<ObjectInfo>, which sliced any subclass (e.g. IcebergDataObjectInfo) down to the base ObjectInfo. This caused Iceberg cluster table functions with cluster_table_function_split_granularity = 'bucket' to lose Iceberg-specific metadata (manifest entry, schema ID, delete files), leading to a failed assertion in IcebergSource::createReader.

Fix by adding a virtual clone method to ObjectInfo and overriding it in IcebergDataObjectInfo. Use clone in ObjectIteratorSplitByBuckets::next instead of copy-constructing into a base-class shared_ptr.

Workflow [PR], commit [0173cf5]

AI Review

Summary

This PR fixes the reported Iceberg slicing bug in ObjectIteratorSplitByBuckets::next by switching the per-bucket copy to polymorphic clone, and the new integration test is a good regression for lost position-delete metadata. I do not think it is merge-ready yet because the archive-backed bucket-splitting path that this change now advertises via ObjectInfoInArchive::clone is still not implemented end to end.

PR Metadata

• Changelog category: Bug Fix is correct.
• Changelog entry: the current text overstates the fix by saying the same path also affects archive-backed reads and ObjectStorageQueue objects. ObjectStorageQueue does not go through ObjectIteratorSplitByBuckets, and archive-backed cluster reads are still broken for bucket granularity.
• Suggested replacement: Fix object slicing in bucket splitting of Iceberg cluster table functions (cluster_table_function_split_granularity = 'bucket'). Previously the per-bucket object was copy-constructed into the base ObjectInfo, slicing away IcebergDataObjectInfo metadata and losing the manifest entry, schema id, and delete files, which could return wrong results or raise an exception in IcebergSource.

Findings

⚠️ Majors

• [dismissed by author -- https://github.com/Fix object slicing in ObjectIteratorSplitByBuckets::next #100819#discussion_r3331105431] [src/Storages/ObjectStorage/IObjectIterator.cpp:169, src/Storages/ObjectStorage/StorageObjectStorageSource.cpp:1848] ObjectInfoInArchive::clone preserves the dynamic type, but archive-backed cluster reads with cluster_table_function_split_granularity = 'bucket' are still not valid end to end. The splitter still opens last_object_info->relative_path_with_metadata, which is empty for ObjectInfoInArchive, and the worker-side archive reconstruction still recreates the member from path/read-source-index only, dropping any file_bucket_info. So this PR still leaves archive bucket tasks in the same broken state while the new override and changelog imply they are covered.
  Suggested fix: either handle archive members explicitly in ObjectIteratorSplitByBuckets::next and ReadTaskIterator::createObjectInfoInArchive while carrying file_bucket_info through reconstruction, or reject bucket granularity for archive-backed cluster functions until that path is implemented.

Tests

• ⚠️ If the intended scope includes archive-backed cluster functions, add one focused integration test for cluster_table_function_split_granularity = 'bucket' with cluster_function_process_archive_on_multiple_nodes = 1. If the intended scope is only Iceberg, add a negative test that archive-backed cluster functions reject bucket granularity instead of silently taking the broken path.

Final Verdict

• Status: ⚠️ Request changes
• Minimum required actions: Either complete the archive-backed bucket path end to end, or explicitly reject it and narrow the PR/changelog scope to the Iceberg case that is actually fixed and tested.

The AST fuzzer (arm_asan_ubsan) failure is a known flaky UBSan issue in BSINumericIndexedVector. Fix: #100086.

Resolved the merge conflict with `master` (the new `has_cache_entry` / `filterByMatchingRowGroups` logic in `ObjectIteratorSplitByBuckets::next` is now combined with the polymorphic `clone`), fixed the changelog entry, and added a regression test.

On the review feedback:

• The slicing concern for other subtypes is already covered: this PR overrides `clone` in `IcebergDataObjectInfo`, `ObjectInfoInArchive`, and `ObjectStorageQueueObjectInfo`, so none of them are sliced in this path.
• Added test_cluster_table_function_split_by_buckets_with_position_deletes to tests/integration/test_storage_iceberg_with_spark/test_cluster_table_function.py. It builds a format-version 2 Iceberg table with merge-on-read position deletes and small parquet row groups (so a data file is split into multiple buckets) and asserts that a cluster query with cluster_table_function_split_granularity='bucket' still applies the deletes — this reproduces the reported bug before the fix.
• I did not add a dedicated non-Iceberg bucket-splitting test: ObjectStorageQueue never goes through ObjectIteratorSplitByBuckets (it is only constructed in StorageObjectStorageCluster), so a bucket-split test there is not meaningful. The archive path is fixed via the ObjectInfoInArchive::clone override.

Merged master into the branch (it was ~1400 commits / well over a week behind). The only conflicts were additive in ObjectInfo / IcebergDataObjectInfo, where master had added getFileSizeHint next to the lines this PR adds clone to — kept both. The surrounding has_cache_entry / filterByMatchingRowGroups logic in ObjectIteratorSplitByBuckets::next auto-merged and still uses the polymorphic clone.

Verified the four affected translation units compile cleanly after the merge: IObjectIterator.cpp, IcebergDataObjectInfo.cpp, StorageObjectStorageSource.cpp, ObjectStorageQueueSource.cpp. The diff against master remains exactly the 6 PR files (no submodule drift). CI will re-run on the fresh merge commit.

Merged master into the branch (it was ~590 commits / 3 days behind and the only red was a BuzzHouse failure).

The BuzzHouse (arm_asan_ubsan) failure was not caused by this PR. The fuzzer client exited with code 241 from the injected fault point tcp_handler_fail_connection_setup (Fail point tcp_handler_fail_connection_setup is triggered. (MEMORY_LIMIT_EXCEEDED)) while reconnecting; the server stayed up (server_died=0, Server exit code is 0), and there is no sanitizer report, no crash, and no symbol from the code this PR touches. BuzzHouse error/failure was widespread on master over the last week (≈134 PRs), confirming a harness/fault-injection flake rather than a regression.

This is fixed on master by #107689 (BuzzHouse: do not enable the tcp_handler_fail_connection_setup fail point, commit 294dc6cf186), which landed after this branch's previous merge-base and is now pulled in by this merge, so CI should re-run clean.

The merge was clean: master did not touch any of the 6 files this PR changes, so the net diff against master is unchanged (the polymorphic clone on ObjectInfo and its use in ObjectIteratorSplitByBuckets::next are preserved). No rebuild was needed since the PR's code is byte-identical to the previously-tested state.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 3/19 (15.79%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report

Merged master into the branch (it was ~1 week / ≈1350 commits behind and the only red was a flaky upgrade check).

The merge was a clean auto-merge even though master had meanwhile changed 3 of the touched files:

• IObjectIterator.cpp / IObjectIterator.h: master reworked getIdentifier (now getIdentifier(bool include_file_bucket_info) + getIdentifierForPath, and the query-condition-cache key uses getIdentifier(/*include_file_bucket_info=*/ false)). This is in a different region of ObjectIteratorSplitByBuckets::next than this PR's per-bucket clone() loop, so both were kept.
• StorageObjectStorageSource.h: master added read_source_index (plumbed through RelativePathWithMetadata / createObjectInfoInArchive), readBuffer(), and match_web_paths_only. The polymorphic clone() overrides copy-construct the subtype (std::make_shared<Subtype>(*this)), so the new read_source_index is preserved on cloned per-bucket objects — no slicing regression.

Verified the 4 affected translation units compile cleanly after the merge: IObjectIterator.cpp, StorageObjectStorageSource.cpp, IcebergDataObjectInfo.cpp, ObjectStorageQueueSource.cpp. The net diff against master remains exactly the 6 PR files.

The previous Upgrade check (amd_release) red was not caused by this PR: it flagged Error message in clickhouse-server.log for Code: 395 ... FUNCTION_THROW_IF_VALUE_IS_NON_ZERO raised by a background MutatePlainMergeTreeTask (if(1, _CAST(plus(x, throwIf(1)), 'UInt64'), x)) — a leftover stateless-test mutation, not anything in the object-storage iterator path. This error was widespread on master over the preceding week (≈14–19 PRs/day, including PRs with base_ref = master), confirming a known master-wide flake. CI will re-run on the fresh merge commit.