Antalya 26:6 Multiple fixes for Iceberg operations - #2157
Conversation
subkanthi
changed the title
|
ADD COLUMN DROP COLUMN RENAME COLUMN |
| { | ||
| switch (type->getTypeId()) | ||
| { | ||
| case TypeIndex::UInt8: |
There was a problem hiding this comment.
Added support for bool
| return {"string", true}; | ||
| case TypeIndex::UUID: | ||
| return {"uuid", true}; | ||
| case TypeIndex::Decimal32: |
There was a problem hiding this comment.
Added support for iceberg decimal types.
….com/Altinity/ClickHouse into antalya_26_6_fix_alter_table_iceberg
|
Decimal support |
…data, reject decimal with precision > 38, fix modify column
CI triage for #2157 (
|
| Check | Class | Cause |
|---|---|---|
| Regression release iceberg_2 | 🔴 PR-caused | Whole export partition feature broken |
| Regression release settings | 🟡 pre-existing | Fails on base too; unrelated settings |
| Regression release s3_export_part | 🟡 pre-existing | Fails on base too |
| SQLLogic test | 🟡 pre-existing/infra | Fails on base too; run under-executed |
| Regression release s3_aws_s3_2 | ⚪ flaky/infra | Single retry-level error, unrelated code |
| Regression release tiered_storage_minio | ⚪ flaky | Single scenario, non-Iceberg code |
| Stateless (amd_debug, parallel) | ⚪ flaky | Passed on rerun; non-blocking |
| Stateless (amd_debug, distributed plan, s3 storage) | ⚪ flaky | Unrelated test; non-blocking |
| PR (aggregate) | — | Gate reflecting the above |
🔴 PR-caused — must fix before merge
Regression release iceberg_2 — job
- This PR:
207 features (79 ok, 97 failed, 31 skipped),258 scenarios failed. - Base
antalya-26.6(same--only ".../export partition/*"scope):188 features (112 ok, 76 skipped),1892 scenarios (1851 ok, 0 failed)→ green. - 100% of the failures are under
/iceberg/export partition/…, and they fail across every catalog backend —no catalog,ice catalog, andglue catalog:✘ /iceberg/export partition/ice catalog/plain merge tree/manifest integrity/each export advances the snapshot list by one ✘ /iceberg/export partition/glue catalog/plain merge tree/transactions/sequential exports append one append-snapshot each ✘ /iceberg/export partition/no catalog/plain merge tree/transactions/… ✘ …/manifest integrity/snapshot summary total-records matches exported row count - The scenarios fail fast (≈0.5–1.7 s) on snapshot/metadata assertions (append-snapshot count, snapshot-list length, snapshot-summary
total-records). That pattern — every export scenario tripping early on the snapshot the export produces, regardless of catalog — points at the Iceberg metadata/snapshot generation path, which this PR rewrites substantially:src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp(rewritten several times across the 19 commits — 143-, 120-, 63-line hunks)src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cppsrc/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp,Compaction.cpp
Suggested next steps (I can't pin the exact line from CI alone — see caveat below):
- Pull the per-scenario diff (expected vs actual snapshot/metadata) from the report — it isn't in the GHA log, only in the artifact:
iceberg2 report ·
fails.log.txt ·
nice-new-fails.log.txt - Because export-partition is broken uniformly (not just the alter/catalog cases this PR targets), the regression was almost certainly introduced by one of the later
MetadataGenerator/Mutationsrefactor commits rather than the alter feature itself. Reproduce locally with just:
--only "/iceberg/export partition/no catalog/plain merge tree/transactions/*"and bisect those commits. - Confirm the emitted
metadata.jsonstill writes a correctsnapshotslist /snapshot-log/ summarytotal-recordsafter the refactor — that's what the failing assertions check.
🟡 Pre-existing (red on antalya-26.6 as well — not this PR)
Regression release settings— 6default valuesmismatches, none of them settings this PR touches:
ai_function_embedding_default_credentials,ai_function_text_default_credentials,analyzer_compatibility_apply_final_to_all_joined_tables,export_merge_tree_partition_retry_initial_backoff_seconds,export_merge_tree_partition_retry_max_backoff_seconds,object_storage_propagate_credentials_to_other_storages. Classic testflows default-value reference drift. Basesettingsjob = failure.Regression release s3_export_part—S3Export (part)also = failure on the base run. Pre-existing.SQLLogic test— failure reason istotal tests 4,951,919 < minimum 5,939,581: the run executed too few tests (timeout/under-run), not a content failure. BaseSQLLogic test= failure too. Infra/pre-existing — safe to re-run.
⚪ Flaky / infra (green on base, single scenario, outside this PR's code)
Regression release s3_aws_s3_2— one[ Error ]in/s3/aws s3/part 2/orphans/full replication/detach and drop/run #3(a retry, replication-cleanup timeout). Unrelated to Iceberg. Re-run.Regression release tiered_storage_minio— one[ Fail ]in/tiered storage/with minio/alter table policy(8 s). That's MergeTree storage-policy ALTER, a different code path from this PR's Iceberg ALTER. Unrelated; re-run.Stateless (amd_debug, parallel)—00060_move_to_prewhere_and_setsfailed once (308 s) then: "All reruns passed. The failure is not reproducible (likely a transient issue)". Job exited 0 (non-blocking).Stateless (amd_debug, distributed plan, s3 storage, parallel)—03519_storage_url(URL table function; nothing to do with Iceberg). Job exited 0 (non-blocking).
Caveat: I triage from CI evidence only — I can't build ClickHouse or run the suites in this environment, and I can't fetch the S3 artifact bodies, so I can't quote the exact expected/actual snapshot diff or name the precise broken line. The classification above is grounded in the base-vs-PR job comparison and the failing-scenario names; the exact fix should be confirmed against the fails.log.txt / report linked above. Happy to draft the fix as a separate blau/* PR (or, if you want it committed straight onto antalya_26_6_fix_alter_table_iceberg, say so) once the root-cause line is identified.
🤖 automated CI triage by @blau-ai
|
Hmm, that CI Triage is a bit inaccurate. All plain merge tree fails in the export partition suite are expected. The other failures on replicated merge tree are worth looking at though. (I will take a close look later today.) I also just updated the regression tests release branch to hold the fixes to a lot of failing tests. The next CI run should be less red. UPDATE After close inspection:
Other replicated merge tree fails are due to outdated tests or old issues whose tests should've been skipped on this build. |
…placed throw with log message when table-uuid is missing
…d and replaced throw with log message when table-uuid is missing" This reverts commit 71faeb8.
`OPTIMIZE TABLE` on an Iceberg table started failing when any file in the
table's `metadata/` directory lacked `table-uuid`:
Code: 36. DB::Exception: Table UUID is not specified in some metadata
files for table by path .../metadata/1-0000...-0000.metadata.json.
(BAD_ARGUMENTS)
`table-uuid` is optional at `format-version` 1, so a leftover v1 metadata
file - from before the table was upgraded, or from an external engine that
omits the optional field - was enough to break compaction on an otherwise
healthy table. `SELECT`, `INSERT` and `ALTER TABLE ... ADD COLUMN` on the
same table were unaffected.
The cause was `getPlan` opting into table-UUID-based metadata file
selection, which parses every metadata file in the directory and requires
the field on each one. It was the only call site of
`getLatestOrExplicitMetadataFileAndVersion` that did so; all 15 others use
the lenient default. Besides the hard failure it also turned metadata
selection from a filename-only comparison into O(N) object storage reads
per `OPTIMIZE`.
Restore the previous behaviour at that call site and drop the
`select_by_table_uuid` parameter, which now has no callers. Metadata
selection in `getLatestMetadataFileAndVersion` is unchanged, so the
pre-existing strict path used by the `iceberg_metadata_table_uuid` setting
keeps working exactly as before.
This reverts the metadata selection part of the change; the preceding
commit reverted the follow-up that had turned the resulting exception into
a debug message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s branch
The test issues `OPTIMIZE TABLE ... MANIFEST` with
`iceberg_manifest_min_count_to_compact`, and neither exists here:
OPTIMIZE TABLE t MANIFEST
-> Code: 62. Syntax error: failed at position 32 (MANIFEST). Expected one
of: ... PARTITION, DRY RUN, FINAL, FORCE, DEDUPLICATE, CLEANUP ...
(SYNTAX_ERROR)
SELECT 1 SETTINGS iceberg_manifest_min_count_to_compact = 2
-> Code: 115. Unknown setting 'iceberg_manifest_min_count_to_compact'.
(UNKNOWN_SETTING)
Manifest-only compaction is not part of `antalya-26.6`.
`tests/integration/test_storage_iceberg_multistorage/test.py` already
records this with an explicit `pytest.mark.skip` on
`test_optimize_manifest_with_external_manifest_list` for the same reason, so
the test would have failed on its first statement had the integration suite
run.
It was added to cover the table-UUID-based metadata file selection in
`getPlan`, which the previous two commits reverted, so there is nothing left
for it to exercise. Drop it rather than skip it; it can come back with the
feature it tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`snapshots`, `metadata-log` and `snapshot-log` are optional in the Iceberg
spec, so table metadata written by another engine may omit any of them -
typically for a table that has never been written to. `INSERT` into such a
table leaked a raw Poco exception as `Code: 1000 (POCO_EXCEPTION)`:
Exception: Can not extract empty value (`snapshots` absent)
Poco::NullPointerException (`metadata-log` absent)
Poco::NullPointerException (`snapshot-log` absent)
Five accesses assumed the fields are present:
* `getMaxSequenceNumber` and `getParentSnapshot` extracted `snapshots`
from an empty `Poco::Dynamic::Var`, which throws instead of yielding a
null array. Both now treat an absent array as "no snapshot history":
sequence number 0 and no parent snapshot. `getParentSnapshot` already
returns `nullptr` when no snapshot matches, and every caller guards on
that, so no new null path is introduced.
* The `snapshots`, `metadata-log` and `snapshot-log` appends in
`generateNextMetadata` dereferenced the null array returned by
`Poco::JSON::Object::getArray` for a missing key. They now go through
`getOrCreateArray`, which creates an empty array first - the same
approach already used for `refs` and `properties` a few lines below.
`getMaxSequenceNumber` returns early on `last-sequence-number`, which is
required at format-version 2, so only a spec-violating table reaches its
`snapshots` read; it is guarded for consistency rather than for a reachable
failure.
This makes `test_insert_into_table_without_optional_metadata_arrays` pass.
That test was added by c72983e together with the `sort-orders` and
`partition-specs` guards for `DROP COLUMN`, as part of one sweep over
absent optional metadata fields - but the write-path half of the sweep was
never implemented, so the test has been asserting behaviour that did not
exist. It also needed `io` and `get_file_contents` imported, which are
added here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…UMN contracts Three behaviours introduced by this pull request had no unit coverage. `decimal` precision. The Iceberg spec caps `decimal(P, S)` at precision 38 while ClickHouse `Decimal256` reaches 76, so wider precisions have to be refused rather than written into metadata other engines would reject. The tests sweep every precision from 1 to 38 and assert the resulting `decimal(P, S)` string, then assert rejection for 39, 50 and 76. Both sides of the boundary come from a named constant carrying the spec limit, so moving the check in `getIcebergType` fails the tests in both directions. `isModifyColumnApplied`. Five cases covering what the predicate is for: a type already in the schema, a type not yet there, the `required` versus nullable distinction, a column absent from the schema, and the case the predicate exists for - a catalog that applied `Int32` -> `UInt64` and reported the commit as failed, where Iceberg records both as `long` and a retry has to recognise the change as already present. `MODIFY COLUMN` rejection. A change Iceberg cannot record must be rejected *and* leave the metadata untouched, because the original defect was a silent no-op that let the caller persist a ClickHouse schema the Iceberg metadata did not reflect. `expectModifyRejected` asserts the error code and that `current-schema-id` and the `schemas` array are unchanged, for an indistinguishable primitive (`Int32` -> `UInt32`) and an indistinguishable nested type (`Tuple(a Int32)` -> `Tuple(a UInt32)`). Two positive controls keep the suite honest: a no-op MODIFY adds no schema, and a widening `Int32` -> `Int64` adds one schema, moves `current-schema-id`, and records `long` for the field in the schema that id points at. The stored schemas are described directly with a `makeMetadataWithField` helper rather than produced by calling the generator, so the tests check what the Iceberg metadata says instead of whether two functions in this class agree with each other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`DropColumnRejectsIfInSortOrder` and `DropColumnRejectsIfInPartitionSpec` asserted only `EXPECT_THROW(..., DB::Exception)`, which passes for any exception raised anywhere in the call - a `getValue` failure on malformed fixture data, or a `LOGICAL_ERROR` from an unrelated code path, would both have satisfied it. They also said nothing about whether a half-applied schema was left behind, which is the reason rejecting matters: the caller must not go on to persist a ClickHouse schema that the Iceberg metadata does not reflect. Both now go through `expectDropRejected`, which asserts that the call throws, that the code is `BAD_ARGUMENTS`, and that `current-schema-id` and the `schemas` array are unchanged afterwards - the same contract `expectModifyRejected` already checks for `MODIFY COLUMN`. `ModifyColumnRejectsIndistinguishableType` is removed: it covers the same scenario as `ModifyColumnRejectsIndistinguishablePrimitiveType` (`int` stored, `UInt32` requested) while asserting strictly less. `SchemaState`, `readSchemaState` and `expectSchemaUnchanged` move to the first anonymous namespace so the earlier tests can use them, instead of duplicating the comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mkmkme
changed the title
|
Fixed the issues raised during the code review phase. Also changed the title and the changelog entry since those no longer described what the PR does exactly. I also unchecked sanitizers tests so those are not skipped anymore. Let's see how the CI goes |
|
One of Claude's notes: Follow-ups, deliberately out of scope. Iceberg's |
CI triage for
|
| Check | What happened | Classification |
|---|---|---|
Integration tests amd_msan 4/10, amd_tsan 1/6, amd_tsan 3/6 |
Test execution was interrupted (exit status: 2) after 3h11m–3h19m |
Infra — hit the --session-timeout=10800 (3h) cap; no test verdict produced |
Integration tests amd_msan 5/10 |
test_distributed_index_analysis::test_primary_key, test_connection_reuse |
Non-Iceberg, unrelated to the diff — flaky/pre-existing |
Stateless amd_msan (WasmEdge) 2/4 & 3/4 |
04327_png_terminal_mode, 03572_export_merge_tree_part_limits_and_table_functions, 02439_merge_selecting_partitions |
Non-Iceberg — flaky/pre-existing |
Stateless amd_debug (s3) |
04002_deterministic_filter_chain_partition_pruning_key_condition |
Non-Iceberg — flaky/pre-existing |
Stateless amd_asan_ubsan (distributed plan) 2/2 |
Server died (0 test failures) |
Non-Iceberg crash — flaky/pre-existing; safe to re-run |
Stress test (amd_asan_ubsan) |
Cannot start clickhouse-server |
Infra — server startup, not a test result |
SQLLogic test |
total tests 4,901,835 < minimum 5,939,581 |
Infra/threshold — the run didn't execute enough tests (harness/timeout), not a correctness failure |
Regression release settings |
1699/1704 ok, 1 failed | Non-Iceberg TestFlows — flaky/pre-existing |
Regression release alter_attach_1 |
11 failed scenarios (alter/attach_partition) | Non-Iceberg TestFlows — flaky/pre-existing |
GrypeScanServer (-alpine) |
1 high/critical CVE in the base image | Infra/security scan — base-image CVE, not this PR's code |
Recommended re-runs after the decimal fix: the timed-out msan/tsan integration shards and the "Server died"/"Cannot start server" jobs are worth a plain re-run — they show no PR-attributable failure.
Bottom line
Fix the Decimal(10,2) ADD COLUMN round-trip (the added test is correctly asserting 123.40). test_schema_inference is #2216 and outside this PR. Everything else is infra/flaky on paths the PR doesn't touch.
Note: I can't build or run ClickHouse in this environment — this triage is from the praktika S3 reports and static reading of the diff. Correctness of any fix will be confirmed by CI.
CI triage for #2157 (head
|
| class | count | checks |
|---|---|---|
| PR-caused | 0 | — |
| Pre-existing (also red on base) | 3 | Iceberg test_schema_inference (#2216), Regression … settings, amd_tsan data race + 04327_png_terminal_mode |
| Flaky (passed on auto-rerun) | 3 | 03572_export_merge_tree_part_basic, 04105_system_pause_view, 03469_json_read_subcolumns_combined_2_wide_merge_tree |
| Scanner / infra | — | 2× Grype CVE scan, SQLLogic memory-limit |
The scary-looking one is not ours — Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/8)
test_storage_iceberg_with_spark/test_schema_inference.py — 12/12 params fail, all with ATTEMPT_TO_READ_AFTER_EOF on SELECT * FROM … (the server crashed; the 11 that follow only get Connection refused because it never came back).
This is exactly the failure already tracked in #2216: an upstream signed-integer-overflow parsing Iceberg Decimal bounds with scale > 18 (IcebergFieldParseHelpers.cpp:147, 10^30 in an int64_t), exposed on our branch by #2145 (merged 2026-08-11), which added an unconditional per-column bounds-parse in IDataLakeMetadata.cpp:157. It's a deterministic crash present on antalya-26.6 since 2026-08-11.
- Not this PR: Antalya 26:6 Multiple fixes for Iceberg operations #2157 touches neither
IcebergFieldParseHelpers.cppnorIDataLakeMetadata.cpp, andSELECT *doesn't hit any code it changed. - Fix lives in antalya-26.6: test_schema_inference kills the server on amd_asan_ubsan — upstream Decimal-bounds overflow exposed by #2145 #2216 (2-line
scale > 18 → std::nulloptguard inIcebergFieldParseHelpers.cpp:143, or backport the upstream fix). Once that lands onantalya-26.6and you rebase, shard 4/8 goes green.
Pre-existing, unrelated to the PR
- RegressionTestsRelease / Common (settings) / settings — 1/1704 scenario fails. Also red on base
antalya-26.6(MasterCId42f80ac187). Settings suite; PR touches no settings code. - Stateless (amd_tsan, parallel, 2/2) — "Server died" +
ThreadSanitizer: data raceinQueryStatus::releaseWorkloadResources()(ProcessList.cpp:532). Workload-resource teardown race, unrelated to DataLake. - Stateless (amd_msan, WasmEdge, parallel, 2/4) —
04327_png_terminal_mode: aborts reproducibly (5/5 without randomization) withBAD_ARGUMENTSin the PNG/image output format. A real pre-existing bug, but in image formatting — nothing to do with Iceberg. Worth its own ticket.
Flaky — passed on the automatic rerun
- Stateless (amd_asan_ubsan, distributed plan, parallel, 2/2) —
03572_export_merge_tree_part_basic:Data part '2022_3_3_0' is already being exported (ABORTED); rerun passed 1/1, "not reproducible". MergeTreeEXPORT PARTfeature. - Stateless (amd_msan, WasmEdge, sequential, 2/2) —
04105_system_pause_view: refreshable-MV pause/stop result diff; rerun passed. - Stateless (amd_debug, distributed plan, s3 storage, parallel) —
03469_json_read_subcolumns_combined_2_wide_merge_tree: timeout; rerun passed 5/5.
Scanner / infra noise
- Grype Scan (server / server-alpine /
GrypeScanServer) — container CVE scans; the alpine image reports 1 high/critical from the base image. No Dockerfile/dependency change in this PR. - SQLLogic test — the 5 "new" failures are all
Code: 241 memory limit exceeded(RSS ~25.9 GiB) onrandom/select/slt_good_*; runner memory pressure, not a correctness regression. - PR — aggregate gate; red only because of the above.
Bottom line
Nothing here blocks #2157 on its own merits. The only hard failure in the PR's area (test_schema_inference) is the pre-existing #2216 crash introduced by #2145 — it will clear once #2216's Decimal-bounds fix lands on antalya-26.6 and this branch is rebased. Everything else is flaky or infra and safe to re-run.
🤖 automated CI triage — evidence: result_pr.json for 907330c, issue #2216, and base-branch MasterCI d42f80ac187. I could not extract the per-node server crash log (the logs.tar.gz tarball is not accessible from this container), but the client-side ATTEMPT_TO_READ_AFTER_EOF + #2216's locally-reproduced UBSan trace pin it down.
5 participants
continuation of work from #1841
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixes
ALTER TABLE ... ADD COLUMN,DROP COLUMN,RENAME COLUMNandMODIFY COLUMNon Iceberg tables, which could fail against a REST catalog --DROP COLUMNof the most recently added column was rejected withInvalid last column ID, and anALTERthat the catalog had in fact applied could come back asColumn already existson retry.BoolandDecimal(P, S)columns can now be used inCREATE TABLEandALTER TABLE ... ADD COLUMN, and aDecimalprecision above the Iceberg limit of 38 is now refused up front. Dropping a column that the table's sort order or partition spec still references is now rejected, as the Iceberg specification requires.CI/CD Options
Exclude tests:
Regression jobs to run: