Antalya-26.6: iceberg v3 multi-arg transforms ,added logic to read source_ids and not throw an error

[subkanthi]

#2316
implementation similar to apache/iceberg-python#3630

Changelog category (leave one):

• New Feature

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

Implement iceberg v3 feature when the transformations are created in multiple columns, source-ids data is stored in the manifest.
This PR will read iceberg tables created with source-ids and not throw an error.

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 [b63e0a2]

[subkanthi]

@blau-ai

[blau-ai]

CI triage for #2318

Verdict: none of the red checks are caused by this PR. This PR changes only 8 Iceberg data-lake C++ files (ManifestFile*, ManifestFileIterator, Utils, ChunkPartitioner, MetadataGenerator, Constant.h, + a gtest). Every failure below is in a code path the diff does not touch, and the two Iceberg-looking ones are already tracked as pre-existing branch bugs.

Breakdown: 2 integration failures — pre-existing & tracked · 1 stateless — flaky · 1 stateless — pre-existing (cas storage) · Regression TestFlows — pre-existing branch state.

---

Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/8) — 12/1102 ❌ NOT this PR (pre-existing, tracked)

All 12 are test_storage_iceberg_with_spark/test_schema_inference.py::test_schema_inference[*]. It's 1 real crash + 11 cascades: [s3-1-True] kills node1 (ATTEMPT_TO_READ_AFTER_EOF), the other 11 then get Connection refused (…:9000).

This is issue #2216 — a signed-integer overflow parsing Iceberg Decimal bounds with scale > 18 in IcebergFieldParseHelpers.cpp:147, reachable on antalya-26.6 since #2145 (merged 2026-08-11). It fails 12/12 on the base branch on every run since 2026-08-11. The faulty code (IcebergFieldParseHelpers.cpp, IDataLakeMetadata.cpp) is not in this PR's diff — same suite name, different code path. → no action for this PR; tracked in #2216.

Integration tests (amd_asan_ubsan, db disk, old analyzer, 7/8) — 1/635 ❌ NOT this PR (pre-existing, tracked)

test_database_iceberg_lakekeeper_catalog/test.py::test_auth_token_profile_events — deterministic assert 0 >= 1 (reproduces on retry). This is issue #2323: a test-only defect introduced by #2222 (merged 2026-09-04) that fails on every integration build of the branch. Unrelated to partition transforms. → tracked in #2323.

Stateless tests (amd_binary, cas storage / cas s3 storage) — 02265_column_ttl ❌ NOT this PR (flaky/infra)

Praktika labelled it flaky (1/38 reruns failed on one job). Failure is a NETWORK_ERROR in MergeTree/DataPartsExchange.cpp relink during a replicated fetch on shared (cas) storage — nothing to do with Iceberg. The check also carries do_not_block_pipeline_on_failure: true. → safe to re-run.

Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 2/2) — 02435_rollback_cancelled_queries ❌ NOT this PR (pre-existing, cas storage)

Reproducible (3/3): a cancelled INSERT isn't rolled back, so system.processes shows an extra query. This is transaction/rollback behavior on Altinity cas (shared) storage — a config the Iceberg-only diff cannot affect. Pre-existing to the cas-storage config.

Regression (TestFlows): cas_alter_attach_1, cas_s3_cache_*, clickhouse_keeper_no_ssl_1, selects, settings ❌ NOT this PR

These suites exercise ALTER/ATTACH, S3 cache, Keeper, settings and selects — none read Iceberg data-lake code. They reflect the branch's existing regression state, not this diff.

---

Bottom line

The PR itself looks healthy from CI's perspective: the changed code compiled in every build and the Iceberg-specific failures are the branch's known pre-existing crashes (#2216, #2323), not regressions from this diff. No fix is needed on this PR to make these checks pass — they need the underlying branch issues resolved (or a re-run for the flaky 02265). If you want, I can re-run the flaky stateless job.

_{Triaged from the praktika S3 report for 880b51f. I can't build/run ClickHouse here, so this is evidence-based from CI artifacts; correctness of the change itself is ultimately validated by green checks once the branch bugs are addressed.}

[subkanthi]

pyiceberg/trino or spark versions must be updated to create integration tests.

[subkanthi]

@blau-ai

[blau-ai]

CI triage for this PR

Verdict: 0 failures caused by this PR. All red checks are pre-existing base-branch failures, known-flaky tests, or CI-infra problems (OOM / S3 connectivity / server-start). The build is green on all amd targets, so the change compiles cleanly. Re-running will not turn CI green until the base-branch issues below are resolved.

Head SHA analyzed: c320405511333c5967a1f3604224665fa450d364. This PR touches only src/Storages/ObjectStorage/DataLakes/Iceberg/* (multi-arg source-ids reader support) — none of the failing tests exercise that code.

---

1. test_writes_decimal_wide_minmax_pruning[s3] / [local] — pre-existing (base branch)

Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/8)

Code: 36. DB::Exception: Iceberg decimal type supports precision up to 38, got 76. (BAD_ARGUMENTS)
(query: CREATE TABLE ... (d128 Decimal(38, 10), d256 Decimal(76, 20), control Int64) ENGINE=IcebergS3(...))

The test (added tests/.../test_writes.py:706 by 9091be4a672, the iceberg-decimal-support merge) creates a Decimal(76, 20) Iceberg table. A later base-branch commit, e3edda0f5ee "Reject Iceberg decimal with precision above 38 again", restored the precision>38 rejection but did not remove/adjust this test. Both commits are ancestors of this PR's HEAD, so this fails identically on antalya-26.6 — the stack trace goes through getIcebergType / createEmptyMetadataFile, code this PR does not touch.

• Not PR-caused. It's a base-branch inconsistency between the decimal-support test and the decimal-reject commit.
• Fix (belongs to the base branch, not this PR): reconcile e3edda0f5ee with the test — either drop the d256 Decimal(76, 20) column from test_writes_decimal_wide_minmax_pruning (the d128 Decimal(38,10) column already exercises the >8-byte min/max path), or gate the test as xfail while precision>38 is rejected. Worth filing/tracking separately.

2. test_auth_token_profile_events — known pre-existing failure (tracked)

Integration tests (amd_asan_ubsan … 7/8) and (amd_asan_ubsan, targeted)

assert refreshed >= 1
E   assert 0 >= 1

Already tracked as #2323 — "test_auth_token_profile_events fails in all integration builds (broken by #2222)". Lakekeeper OAuth token-refresh accounting, unrelated to Iceberg partition transforms.

• Not PR-caused. Safe to ignore here; follow antalya-26.6: test_auth_token_profile_events fails in all integration builds (broken by #2222) #2323.

3. 03742_nested_loop_join_long, 02265_column_ttl — CAS-S3 infra/flaky

Stateless tests (amd_binary, cas s3 storage, parallel)

• 03742_nested_loop_join_long: WriteBufferFromS3 … Connection refused — passed 3/3 on rerun (S3 blip).
• 02265_column_ttl: NETWORK_ERROR … Source did not prove it still holds the manifest it offered … relink abandoned — flaky in the CAS storage path (27/29 reruns failed under the randomized settings, but this is CAS-manifest-relink infra, not query logic).
• Not PR-caused (CAS content-addressable-storage layer; this PR touches no CAS code). Safe to re-run.

4. Stateless (amd_asan_ubsan, cas s3 storage, parallel, 2/2) — infra OOM (474 failures)

Every one of the 474 failures is the same:

Code: 241. DB::Exception: (total) memory limit exceeded: would use ~19.2 GiB … maximum: 19.06 GiB. (MEMORY_LIMIT_EXCEEDED)

Suite-wide out-of-memory on the runner, not per-test logic. Not PR-caused — infra. Re-run (ideally on a fresh runner).

5. Stress test (amd_debug) — infra

Cannot start clickhouse-server
Check failed

Server never started — environment/infra. Not PR-caused. Safe to re-run.

6. Regression: cas_selects, cas_lightweight_delete_2, cas_s3_cache_* — CAS suites

Altinity CAS (content-addressable storage) regression suites. This PR changes no CAS / lightweight-delete / selects code, and these run against the same CAS-S3 environment that is OOMing and dropping S3 connections above. Not PR-caused — same infra/environment class as #3–#4. Safe to re-run.

---

Bottom line

• PR-caused: none. The Iceberg V3 multi-arg (source-ids) reader change looks healthy from CI's perspective — it builds on every amd target and no test that touches the modified code path failed.
• Action items are all outside this PR: (1) the base branch should reconcile the Decimal(76,20) write test with the precision>38 rejection (e3edda0f5ee); (2) test_auth_token_profile_events is antalya-26.6: test_auth_token_profile_events fails in all integration builds (broken by #2222) #2323; (3) the CAS-S3 OOM / connection-refused / server-start failures are infra — re-run those checks.
• Once the base-branch decimal test is fixed and the infra checks are re-run, this PR should go green.

Note: I can't build or run ClickHouse in this environment — this triage is from the praktika S3 reports and the CI logs. Final correctness is validated by CI on re-run.

[subkanthi]

AI audit note: This review comment was generated by AI.

Audit update for PR #2318 (Iceberg v3 multi-argument transforms: reading source-ids)

Scope: only the 8 files changed by this PR at head e29ac08.

Confirmed defects

Medium: Skipping a multi-arg sort field produces a sort key that isn't a prefix of the real order, so read-in-order can return rows in the wrong order

• Impact: For a sort order like [bucket(16, a, b) ASC, c ASC], ClickHouse builds the sorting key (c ASC) but keeps the table's sort_order_id. If every data file is stamped with that sort_order_id, isDataSortedBySortingKey returns true, and the planner treats the files as sorted by c. They are actually sorted by the bucket first. ORDER BY c / ORDER BY c LIMIT n can then return rows in the wrong order or the wrong top-n rows, with no error.

• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp / getSortingKeyDescriptionFromMetadata. It is consumed via IcebergMetadata::getSortingKey → isDataSortedBySortingKey → ReadFromObjectStorageStep::requestReadingInOrder / getDataOrder.

  if (field->has(f_source_ids))
      continue;   // drops this field but keeps the fields after it

• Trigger: A v3 table whose default sort order has a multi-arg field that isn't the last field, and whose data files carry that sort_order_id (as sorted writes produce), queried with ORDER BY on a later sort column.

• Why defect: Dropping a field from the middle of a sort order changes what "sorted" means. The code comment ("safe: data is still correct") holds only when the skipped field is the last one.

• Fix direction: Stop at the first multi-arg field and keep only the prefix before it (break, not continue). If that prefix is empty, return no sorting key.

• Regression test direction: A unit test on getSortingKeyDescriptionFromMetadata with [multi-arg, identity(c)] that expects an empty key, and [identity(c), multi-arg] that expects (c).

Medium: Filtered queries throw an exception on tables that mix a multi-arg partition field with a normal one

• Impact: Any SELECT ... WHERE ... on such a table fails with a std::out_of_range exception, even when the filter doesn't touch a partition column. That contradicts the PR's stated goal of reading source-ids tables without errors.

• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/ManifestFileIterator.cpp / ManifestFileIterator::create: the new multi-arg continue leaves that field out of partition_key_description. The failure surfaces in ManifestFilesPruner::canBePruned (ManifestFilesPruning.cpp, unchanged), which assumes the partition values and the key's types line up one-to-one. partition_key_value still has one value per spec field (AvroForIcebergDeserializer.cpp:197-202), so it is longer than the key:

  const auto & partition_value = entry->parsed_entry->partition_key_value;   // every spec field
  for (size_t i = 0; i < index_value.size(); ++i)
      const auto & type = partition_key->data_types.at(i);                   // only the fields that pass the filter -> out of range

• Trigger: A partition spec such as [bucket[16](a, b), day(ts)], plus any WHERE clause.

• Why defect: This new path makes the table readable only without a filter. The positional mismatch itself predates this PR (it also affects the unsupported-transform and missing-column continues), but multi-arg fields are a new, common way to hit it.

• Fix direction: In ManifestFileIterator::create, record the spec positions that make it into the key, and have canBePruned pick values by those positions instead of assuming they are contiguous.

• Regression test direction: A manifest-pruning unit test (or an integration test with a hand-patched manifest) with [multi-arg, identity(x)] and WHERE x = ..., asserting no exception and correct pruning.

Medium: An empty source-ids array leads to an out-of-bounds vector read

• Impact: A manifest whose partition field has "source-ids": [] reaches source_ids[0] on an empty std::vector. That is undefined behavior: garbage field ids or a crash while reading the manifest.

• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/ManifestFileIterator.cpp / ManifestFileIterator::create

  if (partition_spec_vec.back().isMultiArg())   // size() > 1 is false for size 0
      continue;
  auto source_id = partition_spec_vec.back().source_ids[0];   // out of bounds when empty

• Trigger: A malformed or hand-edited manifest partition-spec with an empty source-ids array.

• Why defect: Metadata from outside ClickHouse must be validated. Every other malformed shape in this branch throws ICEBERG_SPECIFICATION_VIOLATION, but this one reads out of bounds.

• Fix direction: Throw ICEBERG_SPECIFICATION_VIOLATION when source-ids is empty, next to the existing both/neither checks.

• Regression test direction: A unit test that builds a manifest with "source-ids": [] and expects ICEBERG_SPECIFICATION_VIOLATION.

Coverage summary

• Scope reviewed: All 8 changed files: ManifestFileIterator.cpp, ManifestFile.cpp / .h, Utils.cpp, ChunkPartitioner.cpp, MetadataGenerator.cpp, Constant.h, and the gtest. Unchanged code was read only to follow where the changed data goes: partition pruning, delete-file matching, read-in-order, and the display-string callers.
• Categories failed: Read-in-order correctness for sort orders; partition pruning when a spec has fields that can't be pruned on; validation of malformed source-ids.
• Categories passed (7):
  • Write-side guard: ChunkPartitioner rejects multi-arg specs with BAD_ARGUMENTS before any objects are written.
  • DROP COLUMN checks for the active spec and sort order cover source-ids.
  • The both/neither source-id / source-ids checks throw.
  • PartitionSpecsEntry comparison and formatting.
  • Delete-file matching: every spec field is now always included, which also makes specs compare consistently when manifest schemas differ.
  • The SHOW CREATE / display strings are only displayed, never parsed back, and the empty-transform-name case is safe.
  • Concurrency: all of this runs per query on locally owned objects, with no shared mutable state.
• Out of scope (unchanged code, noted only): Compaction.cpp:521 still calls getValue(f_source_id) unconditionally and would throw on multi-arg specs if it were reachable for v3 tables.
• Assumptions/limits: Static analysis only; nothing was built or run. Defect 1 assumes optimize_read_in_order is at its default (enabled) and that files are stamped with the table's sort_order_id, which is typical of sorted writes but depends on the engine. Integration coverage for multi-arg tables doesn't exist yet: the author notes the pyiceberg, Trino and Spark versions need to be updated before one can be written.

[subkanthi]

Audit Report: PR #2318 — Iceberg V3 Multi-Arg Transforms

AI audit note: This review comment was generated by AI (claude-opus-4.6).

Audit update for PR #2318 (iceberg v3 multi-arg transforms: read source-ids, skip pruning/sorting for multi-arg fields, block writes):

Call Graph

Entrypoints:
  1. SELECT on Iceberg table → ManifestFileIterator::create → parse partition spec → pruning
  2. SHOW CREATE TABLE / system.tables → IcebergMetadata::getSortingKey → getSortingKeyDescriptionFromMetadata
  3. SHOW CREATE TABLE → getPartitionKeyStringFromMetadata / getSortingKeyDisplayStringFromMetadata
  4. INSERT into partitioned Iceberg table → ChunkPartitioner → reject multi-arg
  5. ALTER TABLE DROP COLUMN → MetadataGenerator::generateDropColumnMetadata → reject if in multi-arg spec/sort
  6. Partition pruning → ManifestFilesPruner::canBePruned → project partition values by spec positions

Dispatch chain (read path):
  ManifestFileIterator::create
    → parse each partition spec field (source-id vs source-ids)
    → build PartitionSpecsEntry with source_ids vector
    → skip multi-arg from partition_key_ast (continue)
    → record kept spec positions
    → ManifestFilesPruner constructor copies positions
    → canBePruned projects partition_value through positions → KeyCondition::mayBeTrueInRange

Dispatch chain (sorting key):
  IcebergMetadata::getSortingKey
    → getSortingKeyDescriptionFromMetadata
    → break on multi-arg/missing source-id field
    → only attach sort_order_id when key is non-empty
    → isDataSortedBySortingKey checks sort_order_id match

Transition Matrix

Transition	Input State	Output State	Invariant
Parse single-arg partition field	has source-id	source_ids = [id], added to key	source_ids.size() >= 1
Parse multi-arg partition field	has source-ids	source_ids = [id1, id2, ...], skip pruning	isMultiArg() == true, continue
Parse both present	has both	throw ICEBERG_SPECIFICATION_VIOLATION	fail-closed
Parse neither present	neither	throw ICEBERG_SPECIFICATION_VIOLATION	fail-closed
Parse empty source-ids	source-ids: []	throw ICEBERG_SPECIFICATION_VIOLATION	fail-closed
Build sorting key	multi-arg field encountered	break - return prefix only	prefix invariant preserved
Build sorting key	missing source-id	break - return prefix only	prefix invariant preserved
Attach sort_order_id	empty key after break	skip attachment	no false "sorted" claim
Pruning projection	skipped fields	project by recorded positions	index_value.size() == data_types.size()
Write path	multi-arg spec present	throw BAD_ARGUMENTS	writes blocked
DROP COLUMN	column in multi-arg spec/sort	throw BAD_ARGUMENTS	metadata integrity

Confirmed Defects

Medium: ChunkPartitioner multi-arg guard checks source-ids but does not check for missing source-id

• Impact: If a v3 partition spec field has only source-ids (no source-id), the guard correctly rejects multi-arg. But if the field has neither key (malformed metadata from another writer), the code falls through to partition_specification_field->getValue<Int32>(Iceberg::f_source_id) which throws an unhelpful Poco exception instead of a clear ICEBERG_SPECIFICATION_VIOLATION. The ManifestFileIterator::create path correctly handles this case (explicit throw); the write path does not.
• Anchor: ChunkPartitioner.cpp:55-60
• Trigger: Write to a table whose partition spec has a field with neither source-id nor source-ids (malformed metadata from external tooling).
• Why defect: Inconsistent error-contract: equivalent malformed input produces different exception types and messages across the read and write paths.
• Fix direction: Add else if (!partition_specification_field->has(Iceberg::f_source_id)) throw ICEBERG_SPECIFICATION_VIOLATION(...) after the source-ids guard in ChunkPartitioner.cpp.
• Regression test direction: Unit test constructing a spec field with neither key, attempting a partitioned insert.

Low: source_id_to_column_name[source_id] uses operator[] which silently inserts empty string for unknown IDs

• Impact: In getSortingKeyDescriptionFromMetadata (Utils.cpp:1487), if a source-id in a sort-order field doesn't map to any column in the schema (e.g. orphaned metadata after a column rename that didn't update the sort order), operator[] inserts an empty string into the map and returns it. This produces a broken ORDER BY expression like ASC, (empty column name) which either produces a parse error from KeyDescription::parse or a silently wrong key. The read path does not crash, but the error is confusing.
• Anchor: Utils.cpp:1487 — auto column_name = source_id_to_column_name[source_id];
• Trigger: Orphaned sort-order referencing a column ID no longer in the current schema.
• Why defect: operator[] is a write operation on the map; find + early-return/break would be correct.
• Fix direction: Replace with auto it = source_id_to_column_name.find(source_id); if (it == source_id_to_column_name.end()) break; auto column_name = it->second; (consistent with getSortingKeyDisplayStringFromMetadata which uses find).
• Regression test direction: A gtest with a sort-order referencing a non-existent source-id, asserting empty key instead of malformed parse.

Coverage Summary

• Scope reviewed: All 14 files changed by the PR (808 lines of diff). Full call graph from read, write, display, and DDL entrypoints.
• Fault categories executed:
  • Input validation (source-id/source-ids parsing): Passed — both/neither/empty all reject correctly in ManifestFileIterator::create; partial gap in ChunkPartitioner (defect above).
  • Sorting key prefix correctness: Passed — break instead of continue preserves prefix invariant; sort_order_id guard prevents false sorted claims.
  • Partition pruning alignment: Passed — position-based projection ensures index_value and data_types are 1:1; out-of-range guard returns NOT_PRUNED (fail-closed).
  • Write-path rejection: Passed — ChunkPartitioner throws before attempting to evaluate multi-arg transforms.
  • DDL (DROP COLUMN) protection: Passed — both sort-order and partition-spec multi-arg references are checked.
  • Display paths (getPartitionKeyStringFromMetadata, getSortingKeyDisplayStringFromMetadata): Passed — multi-arg fields correctly formatted using formatPartitionFieldDisplayMultiArg.
  • Comparison operator consistency (PartitionSpecsEntry::operator<=>): Passed — vector comparison replaces scalar; correct strong-ordering semantics.
  • Concurrency/thread-safety: Not applicable — no shared mutable state introduced; all new paths are per-request/per-iterator.
  • Memory lifetime: Not applicable — no new raw pointers or lifetime-sensitive patterns; vector<size_t> is value-copied/moved.
  • Exception safety: Passed — all throws happen before state mutation; no partial-update hazard.
• Categories passed: 9 of 11 applicable
• Categories with findings: 2 (input validation, error-contract consistency)
• Assumptions/limits: Static analysis only; no runtime execution. Pruning integration path not tested end-to-end with a real multi-arg manifest (test file is a gtest on sorting keys; full pruning coverage depends on integration tests in CI).

[subkanthi]

---

Here is the audit report for PR #2318:

---

AI audit note: This review comment was generated by AI (Claude Opus 4.6).

Audit update for PR #2318 (Iceberg V3 multi-argument partition/sort transform support):

Confirmed defects:

    Medium: Pruner `partition_key_spec_positions` is not populated when `hasPartitionKey` is true
        Impact: When the pruner is constructed, it reads `partition_key` and
                `partition_key_spec_positions` from the manifest file iterator. However,
                the new `partition_key_spec_positions` field is set via assignment
                (`partition_key_spec_positions = manifest_file.getPartitionKeySpecPositions()`)
                only inside the `if (manifest_file.hasPartitionKey())` block — which is
                correct. But the field is a `std::vector<size_t>` member with default
                empty construction. If by some code path `partition_key_condition` is set
                but `partition_key_spec_positions` is empty (should not happen in current
                code since both are guarded by the same `if`), `canBePruned` would produce
                an empty `index_value` and call `mayBeTrueInRange(0, ...)`, which
                returns true (not pruned). This is fail-closed (safe) so NOT a correctness
                bug — downgraded to observation. No defect.
        Status: NOT CONFIRMED — fail-closed by design.

    Low: `PartitionSpecsEntry` comparison operator (`<=>`) uses lexicographic
         vector comparison on `source_ids`
        Impact: The `<=>` operator on `PartitionSpecsEntry` now compares
                `std::vector<Int32> source_ids` lexicographically. This is used for
                sorting `ProcessedManifestFileEntry` objects (via the `<=>` on
                `ProcessedManifestFileEntryPtr` which includes
                `common_partition_specification`). Changing from `Int32 source_id` to
                `std::vector<Int32> source_ids` changes the sort order of entries.
                For existing V1/V2 tables, `source_ids` always has exactly one element,
                so `vector<Int32>{x} <=> vector<Int32>{y}` produces the same ordering
                as `x <=> y`. For V3 tables with multi-arg fields, the new ordering is
                well-defined and consistent. No correctness break.
        Anchor: ManifestFile.cpp / `operator<=>`
        Status: NOT CONFIRMED — ordering is consistent and correct for both single and
                multi-element vectors.

No confirmed defects in reviewed scope.

Coverage summary:

    Scope reviewed:
        - ManifestFileIterator.cpp: multi-arg parsing, partition_key_spec_positions construction,
          skip-multi-arg-in-key logic
        - ManifestFilesPruning.cpp: projection of partition_value via partition_key_spec_positions,
          bounds-check fail-closed, mayBeTrueInRange size fix
        - ManifestFile.h/cpp: PartitionSpecsEntry struct change, comparison operator, dump formatting
        - Utils.cpp: getSortingKeyDescriptionFromMetadata prefix-break on multi-arg/missing source-id,
          getPartitionKeyStringFromMetadata multi-arg display,
          getSortingKeyDisplayStringFromMetadata multi-arg display,
          formatPartitionFieldDisplayMultiArg helper
        - IcebergMetadata.cpp: sort_order_id guarded by non-empty key check
        - ChunkPartitioner.cpp: early rejection of multi-arg writes
        - MetadataGenerator.cpp: drop-column rejection for multi-arg partition/sort fields
        - Constant.h: f_source_ids alias
        - Unit tests: 2 new test files + extensions to existing test

    Categories passed:
        - Input validation (source-id/source-ids mutual exclusivity, empty array, missing both)
        - Partition pruning projection correctness (positions mapping, bounds check, size argument fix)
        - Sort key prefix truncation (break at first multi-arg, prefix-only guarantee)
        - Drop-column guard for multi-arg references
        - Write-path rejection for multi-arg transforms
        - Display/formatting for multi-arg transforms (bucket, truncate, fallback)
        - Comparison operator consistency (single-element vector backward compat)
        - sort_order_id stale-pairing prevention (empty key → no id)

    Categories not applicable:
        - Concurrency/thread-safety: all changes are in single-threaded parsing/construction paths
        - Memory lifetime: no new ownership patterns; vectors moved into const members
        - Iterator invalidation: no container mutation during iteration

    Assumptions/limits:
        Static analysis only; not executed at runtime. Avro deserialization of multi-arg
        partition tuples assumed to produce one Field per spec field (confirmed by reading
        AvroForIcebergDeserializer.cpp). No V3 multi-arg Iceberg table was available for
        end-to-end testing.

[xieandrew]

Please check the issue related to OPTIMIZE table leaving leftover files.

[xieandrew]

It looks like this could lead to leftover data files when compaction (OPTIMIZE table) is run on a table with source-ids. Compaction.cpp:1385-1395 writes data files before metadata files. writeMetadataFiles at Compaction.cpp:1121 calls the ChunkPartitioner constructor which would throw this exception. So the data files would have already been written but the metadata wouldn't update, leaving those files orphaned.

[subkanthi]

AI audit note: This review comment was generated by AI (Claude Opus 5.5).

Audit update for PR #2318 (Iceberg v3 multi-argument transforms: read source-ids, skip pruning/sorting for multi-arg fields, block writes, reject multi-arg specs in OPTIMIZE before any data file is written), head b63e0a2:

Confirmed defects:

Medium: Catalog SHOW CREATE TABLE throws on tables with a multi-arg partition or sort field
    Impact: For a v3 table in a REST or Glue `DataLakeCatalog` database whose default partition
            spec or sort order has a `source-ids` field, `SHOW CREATE TABLE` (and anything else
            that builds the create query through `DatabaseDataLake::getCreateTableQueryImpl`)
            fails with a raw Poco "Can not convert empty value" exception (`POCO_EXCEPTION`).
            It should report the field as unsupported. `datalake_ignore_unsupported_table_properties = 1`
            cannot get past it, and `throw_on_error = false` callers don't catch it either. That
            contradicts the PR's goal of reading `source-ids` tables without errors.
    Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp / formatIcebergTransformExpression
            (called from getPartitionAndSortingKeyASTsFromMetadata <- RestCatalog::getTableMetadataImpl
            and the GlueCatalog equivalent). This line is unchanged, but it is a call site the PR missed:
            the PR fixed its siblings getPartitionKeyStringFromMetadata and
            getSortingKeyDisplayStringFromMetadata, but not this one.
                const auto it = source_id_to_column_name.find(field->getValue<Int32>(f_source_id));
                // a multi-arg field has no `source-id`, so Var::convert on an empty value throws
    Trigger: `SHOW CREATE TABLE cat.`ns.tbl`` where the default spec is e.g.
             `{"source-ids": [1, 2], "transform": "bucket[16]", ...}`.
    Why defect: The function's documented contract is "return std::nullopt when the field cannot be
                represented", which feeds `unsupported_properties`. A multi-arg field skips that
                contract and throws instead.
    Fix direction (short): Return std::nullopt when the field has `f_source_ids` or lacks `f_source_id`.
    Regression test direction (short): gtest on getPartitionAndSortingKeyASTsFromMetadata with a
                                       multi-arg partition field and sort field, expecting null ASTs and
                                       "PARTITION BY, ORDER BY" in unsupported_properties.

Low: The write-path guard checks only for `source-ids`; a field with neither key still throws a raw Poco exception
    Impact: A spec field with neither `source-id` nor `source-ids` passes
            `checkPartitionSpecSupportedForWrites`. It then fails with a Poco conversion exception
            in the ChunkPartitioner constructor, in `resolve_partition_spec` (manifest compaction),
            and in `verifyIcebergPartitionCompatibility` (export). The read path rejects the same
            input with `ICEBERG_SPECIFICATION_VIOLATION`, so equivalent faults get different error
            contracts. Diagnostics only: every one of these call sites throws before writing, or
            inside a block with cleanup.
    Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/ChunkPartitioner.cpp / checkPartitionSpecSupportedForWrites
                if (partition_specification->getObject(static_cast<UInt32>(i))->has(Iceberg::f_source_ids))
                    throw Exception(ErrorCodes::BAD_ARGUMENTS, ...);
                // no check that `source-id` is present; the caller then does getValue<Int32>(f_source_id)
    Trigger: INSERT, OPTIMIZE or EXPORT PARTITION into a table whose spec field has no source key
             (metadata from external tooling or hand-edited).
    Why defect: An inconsistent error contract between the read and write paths for the same malformed input.
    Fix direction (short): In the helper, throw ICEBERG_SPECIFICATION_VIOLATION when the field has neither key.
    Regression test direction (short): gtest that calls the helper on a field with neither key and expects
                                       ICEBERG_SPECIFICATION_VIOLATION.

Coverage summary:

Scope reviewed: All 19 changed files: ManifestFileIterator, ManifestFilesPruning, ManifestFile,
    Utils (sorting key prefix and display strings), IcebergMetadata::getSortingKey, MetadataGenerator
    (DROP COLUMN), ChunkPartitioner, the Compaction getPlan pre-check and resolve_partition_spec,
    the ExportPartitionUtils pre-check, both gtests, the new stateless test, and the integration
    .gitignore. Unchanged consumers were traced: AvroForIcebergDeserializer partition tuple, the
    INSERT commit/retry cleanup, the Mutations write order, isDataSortedBySortingKey, the
    system.tables display callers, and the REST/Glue catalogs.
Categories failed: catalog/DDL display parity for multi-arg fields (Medium); malformed-spec error
    contract on the write side (Low).
Categories passed: OPTIMIZE orphan prevention (the getPlan check runs before getOldFiles and
    writeDataFiles); every other ChunkPartitioner caller either builds it before writing (INSERT,
    Mutations) or throws inside a cleanup block (manifest compaction, and the INSERT retry, where
    cleanup(true) throws, the outer catch runs cleanup(false), and that deletes the data files);
    the export check runs at request time, before IcebergImportSink writes; pruning projection by
    spec position (the Avro tuple is spec-ordered, 1:1 with data_types, out-of-range fails closed
    to NOT_PRUNED); sorting-key prefix truncation plus suppression of sort_order_id on an empty key
    (ClickHouse INSERT writes no sort_order_id, so no false "sorted" claim); source-id/source-ids
    both/neither/empty validation on read; the DROP COLUMN multi-arg guard; the PartitionSpecsEntry
    <=> change, which is backward compatible for single-element vectors; display strings, which only
    feed system.tables and are never parsed back. Not applicable: concurrency, lock order and
    lifetime (all new state is per query and locally owned; no shared mutable state).
Assumptions/limits: Static analysis only. Nothing was built or run, and 05301 has never been executed
    (the build was aborted), so its reliance on `use_iceberg_metadata_files_cache=0` to see the
    patched metadata is unverified. Not confirmed / residual: compaction rewrites every data file
    under the default spec regardless of the file's own spec (a pre-existing spec-evolution gap, not
    introduced here); the getPlan check dereferences `partition-specs` without a null check, which
    only matters for v2 metadata that already violates the spec (writeMetadataFiles had the same
    pattern already). No end-to-end multi-arg table exists yet: per the author, the
    pyiceberg/Trino/Spark versions need updating first.