Skip to content

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

Open
subkanthi wants to merge 5 commits into
antalya-26.6from
iceberg-v3-multi-arg-transforms
Open

subkanthi wants to merge 5 commits into
antalya-26.6from
iceberg-v3-multi-arg-transforms

Conversation

@subkanthi

@subkanthi subkanthi commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

#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

github-actions Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Workflow [PR], commit [6857a2f]

@subkanthi subkanthi changed the title Added logic to read source_ids and not throw an error Antalya-26.6: iceberg v3 multi-arg transforms ,added logic to read source_ids and not throw an error Sep 6, 2026
@subkanthi

Copy link
Copy Markdown
Collaborator Author

@blau-ai

@blau-ai

blau-ai commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator Author
image pyiceberg/trino or spark versions must be updated to create integration tests.

@subkanthi

Copy link
Copy Markdown
Collaborator Author

@blau-ai

@blau-ai

blau-ai commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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.

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 subkanthi mentioned this pull request Sep 16, 2026
15 tasks
@subkanthi

Copy link
Copy Markdown
Collaborator Author

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants