Antalya 26:6 Multiple fixes for Iceberg operations - #2157
Conversation
|
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 |
|
Could you fix the conflicts please? |
|
Agent found a potential issue: Nested ADD COLUMN + commit-unknown recoveryiceberg_alter_catalog_commit_reported_as_failed makes RestCatalog::updateSchema apply the catalog commit and then return false. The ALTER retry path is supposed to notice the column is already there via MetadataGenerator::isAddColumnApplied and treat the op as succeeded. That works for a primitive ADD COLUMN z Nullable(String) (test_writes_alter_when_commit_is_reported_as_failed). It does not work for nested types. SET allow_insert_into_iceberg = 1, enable_nullable_tuple_type = 1;
ALTER TABLE … ADD COLUMN tuple_col Nullable(Tuple(Int32, String));
Code: 36. DB::Exception: Column tuple_col already exists. (BAD_ARGUMENTS)The catalog already committed the column. Recovery missed it, so the retry called generateAddColumnMetadata again. CauseisAddColumnApplied (MetadataGenerator.cpp ~181–199) rebuilds the expected Iceberg type with getIcebergType(type, last_column_id) using the current last-column-id (already advanced by the successful commit). For primitives that is a string ("string", "long", …) and the compare succeeds. For Array / Map / Tuple, getIcebergType allocates new nested field ids from that high watermark. icebergTypesEqual stringifies the JSON, so element-id / key-id / value-id / struct field ids do not match the committed type → returns false → retry → Column already exists. |
….com/Altinity/ClickHouse into antalya_26_6_fix_alter_table_iceberg
PR CI Triage SummaryIceberg ALTER coverage passed (iceberg_1). None of these are caused by this PR. Same jobs are already red on MasterCI antalya-26.6 iceberg_2 — EXPORT PARTITION tests against a 26.6 binary that does not have that feature. Unrelated to Iceberg ALTER. s3_export_part — same export-feature mismatch, then cascade after the server died. Sanity cases passed. settings — missing snapshots for new 26.6 settings. Already failing on MasterCI. Stateless amd_debug + distributed plan + s3 — three 600s timeouts. Passed on the other stateless jobs in this run. Known flakes. swarms — historically flaky node-restart test. Already failing on MasterCI. tiered_storage_minio — disk-name flake. Passed on the other tiered-storage jobs in this run and on MasterCI. |
…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>
|
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 |
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: